Skip to content

Unit test avoids creating new contexts unless it needs specific configuration options #5475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

rroelke
Copy link
Member

@rroelke rroelke commented Mar 12, 2025

This was at attempt at SC-63578. Performance results indicated that more vCPUs correlates negatively with performance. We speculated that this was because of the time required to spawn and join threads for each context. This branch reduces the number of contexts created by adding functions vanilla_context_c and vanilla_context_cpp which return an existing Context for unit tests which don't need any special configuration.

Ultimately this does not appear to be the cause of the negative correlation. time ./tiledb_unit shows that this branch runs in just under 21 minutes instead of just over 22 on a large EC2 instance with 96 vCPUs, which is only a 5% improvement. That's nothing to write home about, but the work is already done, so we might as well take it.

In addition to the vanilla_context_c and vanilla_context_cpp functions, there is refactoring of the VFSTestSetup class, splitting it into VFSTestContext and VFSTempDir.

The non-trivial changes live in test/support/src/{helpers,vfs_helpers}.{cc,h}.


TYPE: NO_HISTORY
DESC: Create fewer contexts in tiledb_unit

@rroelke rroelke requested review from ypatia and teo-tsirpanis March 12, 2025 14:16
@teo-tsirpanis
Copy link
Member

Hmm, on the one hand I'm not sure if we should merge it if the performance gains are not significant (for comparison, on a two-core machine the tests run one order of magnitude faster), but on the other hand this work will be useful when the C API gets fixed to make contexts thread-safe, but if we wait until then we risk bitrotting the PR.

@bekadavis9 bekadavis9 self-requested a review March 13, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants