Sort test: split and make it more flexible to reduce execution time #2077
Conversation
dmitriy-sobolev
requested review from
MikeDvorskiy,
SergeyKopienko,
mmichel11,
danhoeflinger and
adamfidel
| // TODO: add a test for stability | ||
| #endif | ||
|
|
||
| return TestUtils::done(); |
There was a problem hiding this comment.
Looking to used defines I think it's possible then this test doing nothing.
In this case important to return return TestUtils::done(0);
It's make sense for this test and probably for other added in this PR tests too.
There was a problem hiding this comment.
This combination skips the tests: "test only device policies" (TEST_ONLY_HETERO_POLICIES) and "device backend is not available" (!TEST_DPCPP_BACKEND_PRESENT). I treat it as invalid. I've added a check to stop testing if it happens.
| std::vector<std::size_t> extended_sizes = test_sizes(8'000'000); | ||
| test_sort<std::uint16_t>(SortTestConfig{cfg, "uint16_t, device, custom greater comparator"}, extended_sizes, | ||
| Device<1>{}, Converter<std::uint16_t>{}, ConstGreater{}); | ||
| #if __SYCL_UNNAMED_LAMBDA__ |
There was a problem hiding this comment.
Is there a reason we only care about this name clashing for stable sort with a comparator?
Perhaps this is enough to prevent regression on all APIs, but it relies upon some knowledge of the implementation, and could change (will add JIT time to add this to the other tests).
The main motivation is to reduce the execution time of the test as it takes too much time.
The PR splits sort.pass into four files: stable/unstable sort, interfaces with/without a comparator.
It also more flexibility through filtering into testing to avoid exponential growth of configurations. See
SortTestConfiguses, and the table below showing execution times.It also improves diagnostics, for example:
sort.pass.cpp:216 - wrong result from sort without predicate #2(note: it is misleading - it can be with a predicate).utils_sort.h:307 - float, device, custom greater, total size = 50718, mismatch with reference at index 38847The test was also refactored:
::was removed beforestd::_prefix was removed (no uglification needed in the tests)test_default_name_genfunction was simplifiedBelow are measurements of ctest times of the sort test(s).
mainincludessort.passtime, andthis PRcombines the times of the added four tests.Note:
utils_sort.hpreserves the history ofsort.pass, but GitHub shows it as a new file (which is probably because of large number of changes). Let me know if the refactoring changes should be rolled back to simplify revewing.