libtest: use binary search for --exact test filtering#154865
libtest: use binary search for --exact test filtering#154865sunshowers wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
added
the
T-libs
This comment has been minimized.
This comment has been minimized.
The test array is sorted by name at compile time. When `--exact` is passed in, use binary search for O(f log n) lookups instead of an O(n) linear scan, under the assumption that f << n (which is true for the most relevant cases). This is important for Miri, where the interpreted execution makes the linear scan very expensive. I measured this against a repo with 1000 empty tests, running `cargo +stage1 miri nextest run test_00` (100 tests) under hyperfine: * Before (linear scan): 49.7s ± 0.6s * After (binary search): 41.9s ± 0.2s (-15.7%) I also tried a few other variations (particularly swapping matching tests to the front of the list + truncating the list), but the index + swap_remove approach proved to be the fastest. Questions: - [ ] To be conservative, I've assumed that test_main can potentially receive an unsorted list of tests. Is this assumption correct?
sunshowers
force-pushed
the
binary-search-test
branch
from
1e41231 to
bbb9e3b
Compare
| // invalidate indexes we haven't visited yet. | ||
| let mut result = Vec::with_capacity(indexes.len()); | ||
| for &idx in indexes.iter().rev() { | ||
| result.push(tests.swap_remove(idx)); |
There was a problem hiding this comment.
I have no feedback here, but I just wanted to say this is the first time I've seen swap_remove being used 👀
And it's really cool how you avoid an allocation/making a copy of the TestDescAndFn (not that you could since I don't see TestDescAndFn implement the Clone trait) through this method since the index order is sorted and deduplicated, so moving the TestDescAndFn items in backward index order would always give you your filtered TestDescAndFn in reverse order. You don't have to worry about the wrong item being taken since it gets replaced by the last item in the Vec, which is not something you are taking anyways.
I'll keep a note of this in mind if I need to do something like this in the future.
| let owned_tests: Vec<_> = tests.iter().map(make_owned_test).collect(); | ||
| test_main(&args, owned_tests, None) | ||
| // Tests are sorted by name at compile time by mk_tests_slice. | ||
| let tests = TestList::new(owned_tests, TestListOrder::Sorted); |
There was a problem hiding this comment.
I think we can keep the test_main(&args, owned_tests, None) invocation here, and just construct the TestList inside test_main(), where test_main() would call test_main_inner(). This would reduce code duplication a bit since both test_main_static() and test_main_static_abort() seem to be the only functions that invoke test_main() and they both have a TestListOrder::Sorted. I might be incorrect though since I notice that test_main() is calling on test_main_with_exit_callback(), which denotes the test list order as unsorted, but I'm not sure why it needs to be marked as unsorted for test_main() when the test_main_static_* functions are seemingly only one using it.
Ideally, I would have prefer test_main() to keep with calling on test_main_with_exit_callback() and not need to introduce a test_main_inner() through, making it take a TestList instead of Vec<TestDescAndFn>. However, I can see that librustdoc/doctest.rs does utilize test_main_with_exit_callback(), though I don't see why a TestList object can not be constructed there.
There was a problem hiding this comment.
Yeah, I definitely thought of that, as well as the related approach of making TestList be a newtype wrapper which always ensures sortedness. Happy to hear opinions before I go about making these bigger changes.
At the moment, I believe that within this repo, merged doctests pass in a potentially-unsorted list. But I wasn't sure how much these methods are callable outside the repo.
5 participants
When
--exactis passed in, use binary search for O(f log n) lookups instead of an O(n) linear scan, under the assumption that f << n (which is true for the most relevant cases).This is important for Miri, where the interpreted execution makes the linear scan very expensive.
I measured this against a repo with 1000 empty tests, running
cargo +stage1 miri nextest run test_00(100 tests) under hyperfine:I also tried a few other variations (particularly swapping matching tests to the front of the list + truncating the list), but the index + swap_remove approach proved to be the fastest.
Questions: