Skip to content
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

Merged
merged 9 commits into from
Mar 5, 2025

Conversation

dmitriy-sobolev
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev commented Feb 24, 2025

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:

  • Before: sort.pass.cpp:216 - wrong result from sort without predicate #2 (note: it is misleading - it can be with a predicate).
  • After: utils_sort.h:307 - float, device, custom greater, total size = 50718, mismatch with reference at index 38847

The test was also refactored:

  • :: was removed before std::
  • _ prefix was removed (no uglification needed in the tests)
  • test_default_name_gen function was simplified
  • some functions were regrouped.

Below are measurements of ctest times of the sort test(s). main includes sort.pass time, and this PR combines the times of the added four tests.

Device configuration main, s this PR, s
GPU, Intel® Data Center GPU Max 1100 release 310 8
GPU, Intel® Data Center GPU Max 1100 debug 1170 40
       
CPU (Intel(R) Xeon(R) Platinum 8480+, 2 sockets) release 286 20
CPU (Intel(R) Xeon(R) Platinum 8480+, 2 sockets) debug segfault at 127s segfault at 14s in sort_comp.pass, 47s the rest combined
       
CPU (Intel(R) Core(TM) i7-1270P) release 373 186
CPU (Intel(R) Core(TM) i7-1270P) debug >3600 390

Note: utils_sort.h preserves the history of sort.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.

Copy link
Contributor

@SergeyKopienko SergeyKopienko left a 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?

@dmitriy-sobolev
Copy link
Contributor Author

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:

  • add a test for a MoveConstructible only type: we do not check if we need only that trait with a device policy.
  • add a test for stability: we lack such checks for host policies without a comparator, and do not check it on device entirely.

I will create issues to highlight it.

@danhoeflinger
Copy link
Contributor

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.

@dmitriy-sobolev dmitriy-sobolev merged commit f49feb4 into main Mar 5, 2025
22 checks passed
@dmitriy-sobolev dmitriy-sobolev deleted the dev/dmitriy-sobolev/split-sort-test branch March 5, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Test only Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants