Conversation
Write the sorted output through a plain buffered writer instead of a line-buffered one, so a pipe or file gets full buffers rather than one write per line. The merge path used to drop its writer without flushing and could lose the last buffer on a write error; flush it explicitly and report the failure with the output name, like the in-memory path does.
The plain whole-line sort went through `compare_by`, which covers every sort
mode and is far too large to inline, and then spent most of its time in
memcmp: every one of the n log n comparisons dereferenced two line pointers
into a buffer hundreds of MB wide, and on inputs like paths or logs walked
tens of identical bytes before finding a difference.
Dispatch once up front and hand the sort a comparator holding only that
case. Give each line a key: the first 8 bytes after the prefix shared by
every line in the chunk, packed big-endian into the `index` field, which the
whole-line sort does not otherwise use. Keys order like the bytes they came
from, so the comparator compares keys first and only reads the lines when
they tie, and then only past the shared prefix. The shared-prefix scan
checks each line with one `starts_with` and stops as soon as the prefix is
empty; merging and checking never sort in memory, so they skip it.
Also on this path:
- lines that tie are byte-identical, so a stable sort cannot change the
output; use the unstable sort even with -s/-u.
- with a single thread, use the std sorts instead of rayon's older ports.
- a key whose `r` disagrees with the global one (`-k1r` without `-r`) stays
off the fast paths, which only know the global `r`.
Measured on 28 cores with LC_ALL=C; `before` is the parent commit, GNU is
9.10. Output is byte-identical to GNU on every input below, with and without
-r/-s/-u.
awk 'BEGIN{srand(7);for(i=0;i<12000000;i++)printf "%09d-key-%s\n", \
int(rand()*999999999), "payload"}' > manylines
awk 'BEGIN{srand(3);for(i=0;i<8000000;i++)printf \
"/very/long/shared/directory/prefix/that/repeats/file-%07d.log\n", \
int(rand()*9999999)}' > prefix.txt
awk 'BEGIN{srand(9);for(i=0;i<20000000;i++)printf "%d\n", \
int(rand()*100000)}' > shortdup.txt
awk 'BEGIN{srand(5);for(i=0;i<3000000;i++)printf "%d%s%09d\n", i%3, \
"x...x" (100 chars), int(rand()*999999999)}' > patho.txt
hyperfine --warmup 1 -n before "$BEFORE $f -o /dev/null" \
-n after "$AFTER $f -o /dev/null" -n gnu "/usr/bin/sort $f -o /dev/null"
manylines, 12M distinct lines, 264 MB:
before 1.957 s user 14.99 s --parallel=1: 6.24 s
after 1.045 s user 2.75 s --parallel=1: 1.99 s
GNU 2.601 s user 8.29 s --parallel=1: 6.56 s
prefix.txt, 8M paths sharing a 54-char prefix, 520 MB:
before 1.970 s user 12.89 s --parallel=1: 5.65 s
after 1.146 s user 2.59 s --parallel=1: 1.91 s
GNU 2.215 s user 6.64 s --parallel=1: 5.46 s
shortdup.txt, 20M short lines, 100k distinct:
before 1.962 s user 15.88 s --parallel=1: 8.35 s
after 1.565 s user 6.05 s --parallel=1: 4.07 s
GNU 3.238 s user 11.01 s --parallel=1: 9.03 s
patho.txt, 3 distinct keys then 100 equal bytes (worst case for the key):
before 0.676 s user 4.14 s --parallel=1: 2.68 s
after 0.727 s user 4.37 s --parallel=1: 2.24 s
GNU 1.039 s user 2.75 s --parallel=1: 2.15 s
Single-threaded we were slower than GNU on prefix.txt and patho.txt; now we
are ahead or level on everything. The wall-clock win is the memory traffic
that the keys avoid rather than fewer instructions: cachegrind puts a
1M-line prefix.txt sort at 1,374,447,050 retired instructions, down from
1,520,263,650 with the specialised comparator alone.
A precomputed collation key per line only pays off when each line takes part in many comparisons. Merging and checking compare each line about once, so they now collate on demand and skip building the key buffers. uucore's `try_init_collator` used to report failure when the collator was already set up, which made a second run in the same process (an embedded utility) fall back to byte order; it now keeps the existing collator and reports that collation is active.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core sorting/ordering and buffered I/O behavior (including locale collation paths), which is correctness-sensitive and warrants careful human validation despite the added tests.
Pull request overview
This PR focuses on improving sort performance while preserving output correctness across common modes (plain sort, merge -m, and check -c), including locale-aware collation scenarios.
Changes:
- Buffer sorted output writes (and ensure buffered output is flushed) to reduce syscalls while still surfacing write failures.
- Speed up whole-line comparisons by precomputing a shared-prefix length and per-line prefix keys, avoiding repeated expensive comparator calls during in-memory sorting.
- For locale collation in
-m/-c, collate on demand instead of precomputing collation keys for every line.
File summaries
| File | Description |
|---|---|
| tests/by-util/test_sort.rs | Adds regression and correctness tests for buffered output, merge/check write failures, whole-line ordering, and locale-collation behavior. |
| src/uucore/src/lib/features/i18n/collator.rs | Makes collator initialization idempotent across repeated runs in the same process, keeping “collation active” semantics. |
| src/uu/sort/src/sort.rs | Implements output buffering, whole-line prefix-key optimization, on-demand collation support, and shared write-failure context. |
| src/uu/sort/src/merge.rs | Ensures merged output flushes buffered writes and reports write failures with the correct output target; uses on-demand collation settings. |
| src/uu/sort/src/ext_sort/threaded.rs | Passes the new “want prefix keys” flag for in-memory chunk sorting in the threaded external sort path. |
| src/uu/sort/src/chunks.rs | Tracks shared prefix length in LineData and conditionally computes prefix keys only when needed for in-memory sorting. |
| src/uu/sort/src/check.rs | Uses on-demand collation settings and avoids prefix-key computation in check mode. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merging this PR will degrade performance by 36.09%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | sort_long_common_prefix_utf8_locale |
2.5 ms | 109.8 ms | -97.71% |
| ❌ | Simulation | sort_very_long_lines_utf8_locale |
150.7 ms | 6,544.2 ms | -97.7% |
| ❌ | Simulation | sort_ascii_utf8_locale |
2.8 s | 11.9 s | -76.93% |
| ❌ | Simulation | sort_spill_to_tmp_files_utf8_locale |
340.2 ms | 1,401.6 ms | -75.73% |
| ❌ | Simulation | sort_reverse_utf8_locale |
82.3 ms | 320.6 ms | -74.32% |
| ❌ | Memory | merge_single_file_utf8_locale |
86 KB | 334.1 KB | -74.26% |
| ❌ | Simulation | sort_unique_utf8_locale |
85.7 ms | 329.9 ms | -74.04% |
| ❌ | Simulation | sort_mixed_utf8_locale |
82.6 ms | 316.2 ms | -73.86% |
| ❌ | Memory | sort_long_line[10000] |
140.2 KB | 342.6 KB | -59.07% |
| ❌ | Simulation | merge_pre_sorted_files_utf8_locale |
251.9 ms | 598 ms | -57.88% |
| ❌ | Memory | sort_spill_to_tmp_files_utf8_locale |
7.7 MB | 17.7 MB | -56.52% |
| ❌ | Memory | sort_long_common_prefix_utf8_locale |
629.9 KB | 1,359.5 KB | -53.67% |
| ❌ | Memory | sort_ascii_utf8_locale |
52.4 MB | 100.6 MB | -47.91% |
| ❌ | Memory | sort_very_long_lines_utf8_locale |
36 MB | 68.3 MB | -47.2% |
| ❌ | Simulation | check_sorted_utf8_locale |
492.2 ms | 860.7 ms | -42.81% |
| ❌ | Memory | sort_reverse_utf8_locale |
2.3 MB | 4 MB | -42.43% |
| ❌ | Memory | sort_mixed_utf8_locale |
2.3 MB | 4 MB | -42.43% |
| ❌ | Memory | sort_unique_utf8_locale |
3.5 MB | 5 MB | -30.18% |
| ❌ | Memory | merge_pre_sorted_files_utf8_locale |
792.4 KB | 1,040.6 KB | -23.85% |
| ❌ | Memory | merge_pre_sorted_files |
910 KB | 1,158.4 KB | -21.44% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sylvestre:sort-perf (e2ac432) with main (9cc5dae)2
Footnotes
-
50 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(8118c24) during the generation of this report, so 9cc5dae was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Contributor
Author
|
oh :) |
sylvestre
marked this pull request as draft
September 8, 2026 21:16
|
GNU testsuite comparison: |
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
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.
Three perf changes to sort: