Replace quicksort with stable merge sort in array.sort() - #629
Merged
Conversation
…sort (H-2) array.sort() degraded to quadratic time on the inputs programs produce most often, and the compiled backend crashed outright on large ones. The compiled runtime used a Lomuto-partition quicksort with a last-element pivot and no randomization, recursing on both halves. Sorted or reverse-sorted input hit the worst case: O(n^2) comparisons and O(n) recursion depth. A presorted 200k-element array segfaulted from stack exhaustion; 2000 elements took ~5ms instead of ~0ms. Sorted input is not an exotic case — it is what incremental workloads such as depth-sorting a scene each frame naturally produce. The interpreter used an insertion sort, which is fine on presorted input but O(n^2) on shuffled input. The two backends also disagreed on equal-comparing elements, since quicksort is unstable and insertion sort is stable. Both backends now use the same stable bottom-up merge sort: - O(n log n) comparisons on every input, with no quadratic worst case. - Iterative, so no array size can exhaust the C stack. - Stable, so both backends agree on ties and sort() gains a documented guarantee it did not previously have. - Each merge first checks whether the two runs are already in order and skips the copy if so, keeping fully sorted input at O(n) comparisons. Measured on a presorted array (compiled backend): 200k elements now sorts in 1ms where it previously segfaulted, and 2000 elements drops from ~5ms to ~0ms. Interpreter: 50k shuffled elements sorts in 7ms. Two correctness properties come with the rewrite. Each merge builds its output in scratch space and copies back only once complete, so a comparator that throws leaves the array a valid permutation of its elements rather than a buffer with duplicated slots and corrupted reference counts. And a comparator that resizes the array being sorted — which would dangle the element pointer the merge is walking — now raises a catchable error instead of a use-after-free. Adds tests/parity/methods/array_sort.hml covering default and custom comparators, stability, presorted/reversed/shuffled input, edge cases, and the throwing-comparator path. Verified clean under valgrind on both backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqFtipMZWUEAWQ8534MuBk
Patch release for the array.sort() fix: bumps HEMLOCK_VERSION to 2.9.1 and records the change in CHANGELOG.md and the version.h history. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqFtipMZWUEAWQ8534MuBk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the O(n²) quicksort implementation in
array.sort()with a stable bottom-up merge sort that guarantees O(n log n) comparisons on every input. This fixes performance degradation on presorted and reverse-sorted arrays, and eliminates stack overflow risk on large arrays.Changes
Interpreter backend (
src/backends/interpreter/io/array_methods.c):sort_default_compare()to centralize default comparison logicsort_compare()wrapper that handles both default and custom comparators, with mid-sort resize detectionsort_merge_runs()to merge two adjacent sorted runs with a fast-path optimization for already-ordered dataarray_merge_sort()implementing iterative bottom-up merge sort with scratch buffer allocationarray_merge_sort()Compiled backend (
runtime/src/builtins_array.c):partition()andquicksort()) withmerge_runs()andmerge_sort()SortContextto track array pointer and expected length for resize detectionDocumentation (
docs/reference/array-api.md):sort()documentation to describe the merge sort algorithm, its O(n log n) guarantee, and behavior when comparators throw or resize the arrayTests (
tests/parity/methods/array_sort.hmland.expected):Implementation Details
<= 0in merge comparisons to take from the left run on ties, preserving original orderpush(),pop(), etc. are caught and throw"sort() comparator resized the array being sorted"rather than causing use-after-freehml_throw()does not unwind)https://claude.ai/code/session_01GqFtipMZWUEAWQ8534MuBk