Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

<algorithm>: Implement worst-case linear-timenth_element#5100

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@muellerj2
Copy link
Contributor

Fixes#856 forstd::nth_element andstd::ranges::nth_element. This implements a fallback to the median-of-medians-of-five algorithm when the quickselect algorithm seems to be making too little progress.

The median-of-medians algorithm is mostly the textbook version, with two minor tweaks:

  • If the processed sequence doesn't cleanly divide into groups of five elements, the remainder group with less than five elements isn't considered for the median computation. (This reduces the amount of code and doesn't make any difference in the asymptotics. I couldn't observe any practical difference in running time, too.)
  • When the pivot (=median-of-medians) has been computed, all (greater) medians located after the pivot are moved to the very end of the processed sequence and the pivot is swapped into the middle of the sequence. This is because all of these elements are guaranteed to be moved by the pivot partitioning algorithm, so this step immediately moves them into an appropriate position (or the pivot probably closer to it). This way, the medians can also be excluded from the sequence on which the partitioning algorithm is applied, avoiding some unnecessary comparisons. (In practice, the benchmarks suggested that this makes the algorithm a few percent faster, but the difference is minor.)

Benchmark results

bm_uniform just appliesnth_element to an integer array of the given length. The integer array is uniformly sampled from a fixed seed. This is to check that the worst-case fallback does not noticeably worsen the processing time on such a sequence.

bm_tunkey_adversary appliesnth_element to a sequence on which the implemented quickselect algorithm performs terribly.

Before:

--------------------------------------------------------------------------------Benchmark                                      Time             CPU   Iterations--------------------------------------------------------------------------------bm_uniform<alg_type::std_fn>/1024           1845 ns         1803 ns       407273bm_uniform<alg_type::std_fn>/2048           3966 ns         3990 ns       172308bm_uniform<alg_type::std_fn>/4096           7702 ns         7673 ns        89600bm_uniform<alg_type::std_fn>/8192          18090 ns        18032 ns        40727bm_uniform<alg_type::rng>/1024              1759 ns         1758 ns       373333bm_uniform<alg_type::rng>/2048              3985 ns         4011 ns       179200bm_uniform<alg_type::rng>/4096              7694 ns         7847 ns        89600bm_uniform<alg_type::rng>/8192             18015 ns        17997 ns        37333bm_tunkey_adversary<alg_type::std_fn>      12995 ns        13393 ns        56000bm_tunkey_adversary<alg_type::rng>         12714 ns        12835 ns        56000

After:

--------------------------------------------------------------------------------Benchmark                                      Time             CPU   Iterations--------------------------------------------------------------------------------bm_uniform<alg_type::std_fn>/1024           1599 ns         1604 ns       448000bm_uniform<alg_type::std_fn>/2048           3626 ns         3610 ns       194783bm_uniform<alg_type::std_fn>/4096           7068 ns         7150 ns        89600bm_uniform<alg_type::std_fn>/8192          16469 ns        16044 ns        44800bm_uniform<alg_type::rng>/1024              1701 ns         1709 ns       448000bm_uniform<alg_type::rng>/2048              3841 ns         3931 ns       194783bm_uniform<alg_type::rng>/4096              7447 ns         7324 ns        74667bm_uniform<alg_type::rng>/8192             17024 ns        16741 ns        37333bm_tunkey_adversary<alg_type::std_fn>       6075 ns         5929 ns        89600bm_tunkey_adversary<alg_type::rng>          6270 ns         6278 ns       112000

As expected, the fallback greatly improves the running time forbm_tunkey_adversary. The timings forbm_uniform are about on par, or more precisely even a bit better with this PR on my machine.

The fallback heuristic

std::sort switches to its fallback when the recursion depth exceeds some logarithmic threshold. We could use the same heuristic as well, however, this would not guarantee linear time in the worst case but "only" an$O(n \log n)$ bound. Alternatively, we could limit the recursion depth to some constant, but that's likely a pessimization for large sequences.

So I opted for an adaptive depth limit: Like the heuristic forstd::sort, it assumes that each iteration should reduce the range of inspected elements by 25 %. But whilestd::sort derives a maximum recursion depth from this assumption, this heuristic falls back to the median-of-medians algorithm when the actual size of the processed sequence exceeds the desired size by some constant tolerance factor (currently about 2) during some iteration. Thus, the total number of processed elements over all quickselect iterations is bounded by a multiple of the sequence length times a geometric sum, ensuring worst-case linear time overall. At the same time, the tolerance factor introduces some leeway so that one or two bad iterations (especially at the beginning) don't trigger the fallback immediately.

Obviously, there are many possible choices for the desired percentage reduction per iteration and the tolerance factor. But the benchmarks seem to suggest that the chosen values aren't too bad; a smaller percentage reduction or a larger margin factor noticeably worsen thebm_tunkey_adversary benchmark, but result in little difference forbm_uniform. Besides, the implementation ofstd::sort already sets a precedent for a desired reduction of 25 % per iteration.

Test

The newly added test appliesnth_element to the same worst-case sequence as the benchmark. This makes sure that the fallback is actually exerted by the test. (I think it's also the first test that exerts the quickselect algorithm and not the just the insertion sort fallback.)

frederick-vs-ja reacted with thumbs up emojiBillyONeal reacted with hooray emoji
@muellerj2muellerj2 requested a review froma team as acode ownerNovember 19, 2024 12:43
@CaseyCarterCaseyCarter added the bugSomething isn't working labelNov 19, 2024
@StephanTLavavejStephanTLavavej added the performanceMust go faster labelNov 19, 2024
@StephanTLavavejStephanTLavavej self-assigned thisNov 19, 2024
@StephanTLavavej

This comment was marked as resolved.

@muellerj2

This comment was marked as resolved.

Copy link
Member

@davidmrdaviddavidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks a lot for this PR ⭐.
Left some comments, mostly nits that I think would improve readability, and some questions.

@muellerj2
Copy link
ContributorAuthor

muellerj2 commentedMar 9, 2025
edited
Loading

New benchmarks with new adversary2 and minor changes to benchmark code (AMD Ryzen 7 7840HS):

Before:

------------------------------------------------------------------------------------------Benchmark                                                Time             CPU   Iterations------------------------------------------------------------------------------------------bm_uniform<alg_type::std_fn>/1024                     1550 ns         1500 ns       448000bm_uniform<alg_type::std_fn>/2048                     3413 ns         3376 ns       203636bm_uniform<alg_type::std_fn>/4096                     6583 ns         6627 ns        89600bm_uniform<alg_type::std_fn>/8192                    15467 ns        15346 ns        44800bm_uniform<alg_type::rng>/1024                        1552 ns         1538 ns       497778bm_uniform<alg_type::rng>/2048                        3494 ns         3299 ns       203636bm_uniform<alg_type::rng>/4096                        6502 ns         6557 ns       112000bm_uniform<alg_type::rng>/8192                       15444 ns        15067 ns        49778bm_tukey_adversary<alg_type::std_fn>/adversary1      11353 ns        11440 ns        56000bm_tukey_adversary<alg_type::rng>/adversary1         11505 ns        11475 ns        64000bm_tukey_adversary<alg_type::std_fn>/adversary2      59287 ns        58594 ns        11200bm_tukey_adversary<alg_type::rng>/adversary2         55013 ns        54688 ns        10000

After:

------------------------------------------------------------------------------------------Benchmark                                                Time             CPU   Iterations------------------------------------------------------------------------------------------bm_uniform<alg_type::std_fn>/1024                     1552 ns         1538 ns       497778bm_uniform<alg_type::std_fn>/2048                     3412 ns         3376 ns       203636bm_uniform<alg_type::std_fn>/4096                     6622 ns         6417 ns       112000bm_uniform<alg_type::std_fn>/8192                    15345 ns        15346 ns        44800bm_uniform<alg_type::rng>/1024                        1458 ns         1444 ns       497778bm_uniform<alg_type::rng>/2048                        3296 ns         3296 ns       213333bm_uniform<alg_type::rng>/4096                        6513 ns         6557 ns       112000bm_uniform<alg_type::rng>/8192                       14922 ns        14753 ns        49778bm_tukey_adversary<alg_type::std_fn>/adversary1       6043 ns         5999 ns       112000bm_tukey_adversary<alg_type::rng>/adversary1          5324 ns         5441 ns       112000bm_tukey_adversary<alg_type::std_fn>/adversary2       4069 ns         4098 ns       179200bm_tukey_adversary<alg_type::rng>/adversary2          3649 ns         3683 ns       203636

@StephanTLavavej

This comment was marked as outdated.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

Thanks! 💚 And apologies for the significant delay in getting around to reviewing this!

I pushed minor cleanups to the product code and moderate refactorings to the test/benchmark code. Please meow if I messed anything up.

I reviewed@davidmrdavid's comments and I believe we've addressed them all.

Click to expand 5950X benchmark results:
BenchmarkBeforeAfterSpeedup
bm_uniform<alg_type::std_fn>/10241877 ns1801 ns1.04
bm_uniform<alg_type::std_fn>/20484190 ns3968 ns1.06
bm_uniform<alg_type::std_fn>/40968131 ns7701 ns1.06
bm_uniform<alg_type::std_fn>/819220427 ns19078 ns1.07
bm_uniform<alg_type::rng>/10241951 ns1736 ns1.12
bm_uniform<alg_type::rng>/20484197 ns3855 ns1.09
bm_uniform<alg_type::rng>/40968120 ns7487 ns1.08
bm_uniform<alg_type::rng>/819225961 ns20594 ns1.26
benchmark_common<alg_type::std_fn>/adversary114572 ns6835 ns2.13
benchmark_common<alg_type::rng>/adversary115813 ns6323 ns2.50
benchmark_common<alg_type::std_fn>/adversary269962 ns4226 ns16.56
benchmark_common<alg_type::rng>/adversary269475 ns4430 ns15.68

@StephanTLavavejStephanTLavavej removed their assignmentApr 23, 2025
@StephanTLavavejStephanTLavavej moved this fromInitial Review toReady To Merge inSTL Code ReviewsApr 23, 2025
@StephanTLavavejStephanTLavavej moved this fromReady To Merge toMerging inSTL Code ReviewsApr 23, 2025
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavejStephanTLavavej merged commitb5df16a intomicrosoft:mainApr 23, 2025
39 checks passed
@github-project-automationgithub-project-automationbot moved this fromMerging toDone inSTL Code ReviewsApr 23, 2025
@StephanTLavavej
Copy link
Member

Thanks for fixing this major bug that went unfixed for decades by some of the world's most experienced C++ Standard Library implementers! 😻 🛠️ 💚

@BillyONeal
Copy link
Member

I wanted to do this for ages, and I'm so happy someone finally has! Thank you so much!

@muellerj2muellerj2 deleted the nth_element-worstcase-linear branchMay 31, 2025 21:48
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@davidmrdaviddavidmrdaviddavidmrdavid left review comments

@StephanTLavavejStephanTLavavejStephanTLavavej approved these changes

Assignees

No one assigned

Labels

bugSomething isn't workingperformanceMust go faster

Projects

Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<algorithm>: nth_element does not comply with worst case running time requirements

5 participants

@muellerj2@StephanTLavavej@BillyONeal@davidmrdavid@CaseyCarter

[8]ページ先頭

©2009-2025 Movatter.jp