-
Notifications
You must be signed in to change notification settings - Fork 115
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
Sort test: split and make it more flexible to reduce execution time #2077
Conversation
test/parallel_api/algorithm/alg.sorting/alg.stable.sort/stable_sort_comp.pass.cpp
Show resolved
Hide resolved
test/parallel_api/algorithm/alg.sorting/alg.stable.sort/stable_sort_comp.pass.cpp
Show resolved
Hide resolved
test/parallel_api/algorithm/alg.sorting/alg.stable.sort/stable_sort_comp.pass.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
But I seen one TODO. Is it ok?
These are the gaps I noticed in the original test, which I do not pursue to fix in this PR. Meanwhile, there are multiple TODOs:
I will create issues to highlight it. |
I have no objections to merging. I was able to review this a decent amount, and believe it is a good change. I haven't had the chance to review to the extent which would warrant hitting "approve", but after some discussion offline about checking code coverage , I think its fine to merge. |
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 to avoid exponential growth of configurations. See
SortTestConfig
uses, 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 38847
The test was also refactored:
::
was removed beforestd::
_
prefix was removed (no uglification needed in the tests)test_default_name_gen
function was simplifiedBelow are measurements of ctest times of the sort test(s).
main
includessort.pass
time, andthis PR
combines the times of the added four tests.Note:
utils_sort.h
preserves 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.