feat(collection): Replace quickSort with pdqsort for performance and robustness #922
Conversation
This commit introduces a comprehensive benchmark harness to provide empirical evidence for the proposal to replace the existing quickSort implementation with a more performant and robust pdqsort algorithm. The harness evaluates performance across five critical data patterns to ensure a thorough comparison: - **Random:** Tests performance on typical, unsorted data. - **Sorted & Reverse Sorted:** Tests for handling of common presorted patterns. - **Few Unique:** Tests efficiency on data with high duplication. - **Pathological:** Tests robustness against inputs designed to cause worst-case behavior in quicksorts. The baseline implementation (`quickSortBaseline`) is a direct copy of the existing SDK code to ensure the comparison is fair and accurate. The results from this benchmark will be used to justify the enhancement in the accompanying pull request.
PR HealthLicense Headers ✔️
All source files should start with a license header. This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with
Breaking changes
|
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| collection | Breaking | 1.19.1 | 1.20.0-wip | 2.0.0 Got "1.20.0-wip" expected >= "2.0.0" (breaking changes) |
This check can be disabled by tagging the PR with skip-breaking-check.
Coverage ⚠️
| File | Coverage |
|---|---|
| pkgs/collection/benchmark/dataset_generator.dart | 💔 Not covered |
| pkgs/collection/benchmark/sort_benchmark.dart | 💔 Not covered |
| pkgs/collection/lib/src/algorithms.dart | 💔 90 % ⬇️ 9 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
This addresses the failing 'Changelog Entry' CI check by adding the required entry for the quickSort implementation replacement.
|
Looks pretty convincing. I'll have to look at it a little more to see if it's so universally better that we want to replace the simple quicksort, or if we may want to have both. I don't know how many people actually use the sort functions in The only nit I noticed in a quick read-through is that I'd avoid calling @natebosch WDYT? |
|
Hi @lrn, thank you for the thoughtful review and feedback! This is a great discussion. Let me address your points directly.
That's the key question. I believe replacing it is the right move because
For these reasons, I don't see a scenario where a user would knowingly choose the older, less robust implementation, which argues against keeping both.
This was an excellent question, and it prompted me to add Benchmark Results: pdqsort (Enhancement) vs. SDK quickSort (Baseline) vs. List.sort
This shows that the goal of this PR isn't to replace
Excellent point, and thank you for catching that! You are absolutely right. I've just pushed a new commit that caches the keys for the pivot and the currently inspected element in the partitioning and insertion sort loops. This will avoid the redundant calls and improve performance when Thank you again for the review! Let me know if you have any other questions. |
This commit refactors the _pdqSiftDown helper function, which is used as part of the heapsort fallback mechanism within the quickSort implementation. The changes improve both performance and readability: 1- Key Caching: The key of the current largest element is now cached in a local variable (largestKey). This avoids multiple redundant calls to the keyOf function within the loop, reducing overhead. 2- Clearer Logic: The comparison logic has been restructured. Instead of potentially re-evaluating keyOf(elements[start + largest]) after the first comparison, the cached key is used, and the flow for comparing the root with its left and right children is now more explicit and easier to follow. 3- Early Exit: An early break is introduced for leaf nodes (when left >= n), which slightly simplifies the control flow. These changes do not alter the algorithm's behavior but make the implementation more efficient and maintainable.
Updated the comment for the BenchmarkResult class to enhance clarity by rephrasing it from "Represents the final aggregated result of a benchmark" to "The final aggregated result of a benchmark." This change aims to improve the readability of the code documentation.
…ckSort and pdqsort This commit introduces a new dataset generator for creating various data patterns used in benchmarking sorting algorithms. It also implements a comprehensive benchmark harness that compares the performance of the baseline quickSort and the enhanced pdqsort across multiple data conditions, including random, sorted, reverse sorted, few unique, and pathological datasets. The results will help evaluate the effectiveness of the pdqsort enhancement.
|
I think this looks fine, so LGTM from me. @natebosch WDYT? |
|
|
||
| List<int> getNextList() { | ||
| // Cloning the list is crucial so each run sorts an unsorted list. | ||
| return List<int>.from(datasets[_iteration++ % datasets.length]); |
There was a problem hiding this comment.
If you clone the list content anyway, there is no need to have 128 different instances of the same list in the sorted/reversed sets. One should be enough.
(This cloning takes time. It's probably unavoidable, we'd have to allocate a lot of lists ahead of time to avoid that, and the memory pressure starts mattering. It should be the same time for all the benchmarks, so at least it's a consistent overhead.
It should use .toList(), not List.from. That's likely to be more performant. The list knows its own length and the fastest way to copy its elements.
I'd not use % with a variable value. That's also expensive. Probably not remotely measurable compared to sorting 50000 elements, though. Still,
could be:
var dataset = datasets[_iteration];
iteration++;
if (iteration == datasets.length) iteration = 0;
return dataset.toList();)
| /// A base class for our sort benchmarks to reduce boilerplate. | ||
| /// Note: We extend `BenchmarkBase` for its structure but will use our own | ||
| /// timing. | ||
| abstract class SortBenchmarkBase extends BenchmarkBase { |
There was a problem hiding this comment.
You can generate the dataset in a setUp function instead of creating it ahead of time.
That would also allow doing benchmarks with varying sizes of inputs, either all small, all large, or mixed.
natebosch
left a comment
natebosch
left a comment
There was a problem hiding this comment.
Overall change looks good. Added a few nitpicks that should be easy to address, I'll handle any others in a followup.
Caches elements and keys in local variables within `_pdqSort3` to reduce `keyOf` calls from a potential 6 down to 3, and to minimize list access during pivot selection.
…llow Effective Dart I removed the leading "get" from method names and updated call sites so the API doesn't start method names with "get". This change follows the Effective Dart guideline: avoid starting a method name with get — see https://dart.dev/effective-dart/design#avoid-starting-a-method-name-with-get
Co-authored-by: Nate Bosch <[email protected]>
Refine the list creation to be more explicit and performant. - Use a more descriptive name `sortedBase` instead of `base`. - Create the initial `sortedBase` list with `growable: false` as it is never modified. - Use `List.of` and explicitly set `growable: true` for the final copies to make the intent clear that they are mutable.
Make the list generation in the `_generateReverse` helper more explicit and performant. - The `base` list is now created with `growable: false`, as it's a read-only template. - The final copies are created using `List.of` with an explicit `growable: true` to clearly signal that they are mutable.
Make the list generation in the `_generateSorted` helper more explicit and performant. - The `base` list is now created with `growable: false` as it's a read-only template. - The final copies are created using `List.of` with an explicit `growable: true` to clearly signal that they are mutable.
Move the generation of `sorted`, `reverse`, and `pathological` base data into top-level `final` variables. - **Efficiency:** Ensures these lists are computed only once at startup. `_generatePathological` now correctly reuses `_sortedBase` instead of re-calculating it. - **Clarity:** Distinguishes between the immutable data templates and the mutable copies used for testing. - **Consistency:** Simplifies the deterministic generator functions into concise one-liners.
Refactor the loops that build `_pathologicalBase` to use `i += 2` for a minor performance and clarity improvement.
Improves code readability.
- Fix a bug in the `_pathologicalBase` creation logic for odd sizes. - Make `_reversedBase` generation more declarative using `.reversed.toList()`. - Add clarifying comments for complex calculations.
4 participants
This PR enhances the performance and robustness of the default unstable sort in
package:collectionby replacing the existingquickSortimplementation with a modern, state-of-the-art Pattern-Defeating Quicksort (pdqsort) algorithm. The implementation is a Dart port of the concepts from the canonical C++ version by Orson Peters.Motivation
The
pdqsortalgorithm is the default unstable sort in other high-performance ecosystems (like Rust) for good reason. It offers two key advantages over the existing implementation:This change is a pure, non-breaking enhancement that makes the standard library sort faster and safer with no API changes or regressions.
Benchmark Results
To validate this enhancement, a comprehensive benchmark was created, comparing the new
pdqsortimplementation against a direct copy of the existing SDKquickSortacross five critical data patterns.The results demonstrate a consistent and significant performance improvement across all tested scenarios, with no regressions.
Benchmark Results:
pdqsort (Enhancement) vs. SDK quickSort (Baseline)
(Results are the average/median time in microseconds to sort 50,000 integers, aggregated over 12 runs.)
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.