-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
sort: improve performance on large chunks #8652
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
base: main
Are you sure you want to change the base?
Conversation
ff0b106
to
7ce1ffe
Compare
Many tasks are failing BTW, could you run the benchmark without your change? |
This, I'm not sure how to fix - the tests are passing, linters are happy - the spellcheck complains about "memrchr", and i10n tests have just timed out. I haven't touched i10n by a large enough margin (I think?) - in what direction should I look at?
Sure thing! We're getting a sample input using dd if=/dev/zero bs=1M count=4096 status=progress | tr '\0' 'A' |
head -c 4294967295 > oneline_4G.txt && echo >> oneline_4G.txt We can't run this exact benchmark using main (as of aaf742d), because the Rust version.. doesn't finish. So, after putting a reasonable timeout (and ignoring exit codes with hyperfine -i "timeout 15s ./target/release/coreutils sort oneline_4G.txt" "timeout 15s sort oneline_4G.txt" --export-markdown report.md
..and with changes to readers:
|
This may be the most advantageous way to fix this problem, for now, but wouldn't it make more sense, in the future, to just store the range of file read (if it is a file, not stdin!) without a newline/separator, instead of writing to a new tmp file, with these extremely large buffers? Wouldn't that avoid writes and be more file cache friendly? |
GNU testsuite comparison:
|
You can fix the cspell errors by adding
to |
bd1d34a
to
b3c9ad9
Compare
CodSpeed Performance ReportMerging #8652 will degrade performances by 3.43%Comparing Summary
Benchmarks breakdown
Footnotes
|
GNU testsuite comparison:
|
What can I do about
in CI? I don't think I made any changes that would break CI in such a way? |
let me rebase it |
it is a huge patch, it is possible to make it smaller for review ? |
GNU testsuite comparison:
|
I mean, I can try? but, eh, we touch a lot of parts that were static before - do we really wanna split these out into multiple merges? the scope of the MR may be reduced, sure (e.g. the reallocation stuff), though! |
most of the jobs are failing, are you going to work on it? thanks |
sure, i will |
- Introduce ReadProgress with SentChunk | NeedSpill | NoChunk | Finished - Cap grow_buffer; read_to_buffer returns NeedSpill on cap without separator
- Maintain up to two in-flight reads - On NeedSpill, stream oversized record to temp run (spill_long_record) - Append single separator; push remainder into carry_over - Preserve ≤2-chunk in-memory fast path
- Use bounded buffer in merge reader - Fallback unbounded read when spill encountered to preserve correctness
(cherry picked from commit 72b70a9)
GNU testsuite comparison:
|
Rationale
sort
does not finish for large one line file #8583) before any output was produced. This change keeps memory bounded while ensuring forward progress by spilling oversized records to temporary runs, aligning behavior with external sort implementations.Overview of changes