sort:Improve fast lexicographic path for sort default mode #9272
+300
−75
Conversation
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
…U sort Adjust the compare_by function to swap the order of byte comparison (b.line vs a.line) specifically in SortMode::Default, ensuring raw byte sorting behavior aligns with GNU sort. This fixes inconsistencies in default sorting order for raw bytes, improving compatibility with expected POSIX behavior.
- Swapped lexicographic comparison from a.line.cmp(b.line) to b.line.cmp(a.line) to correct ordering behavior in compare_by function. - Simplified SortMode::Default from custom_str_cmp with options to plain b_str.cmp(a_str), assuming default sort now uses standard string comparison without special handling for non-printing, dictionary, or case sensitivity. This ensures consistent and correct sorting behavior in the sort utility.
When both lines are valid UTF-8, compare them lexicographically for proper text sorting; otherwise fall back to reversed byte comparison to match GNU sort behavior for raw bytes. Improves sorting accuracy for text data while preserving byte-level sorting for binary input.
- Move fast lexicographic comparison to early check and remove duplication - Replace simple reverse comparison in SortMode::Default with custom_str_cmp function - Ensures proper handling of ignore_non_printing, dictionary_order, and ignore_case options
CodSpeed Performance ReportMerging #9272 will degrade performances by 33.72%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
GNU testsuite comparison: |
- Added token_buffer Vec<Field> to ChunkContents and RecycledChunk - Added utf8_cache Vec<Option<&'a str>> to LineData and RecycledChunk - Updated recycle, read, and parse_lines handling for new fields - Enables efficient tokenization and UTF-8 caching in sort operations
Use clamp() instead of chained max().min() for better readability. This replaces explicit bounds checking in merge_chunk_capacity() with the more idiomatic clamp method, achieving the same effect while reducing code verbosity.
|
GNU testsuite comparison: |
sylvestre
requested changes
Nov 15, 2025
Contributor
sylvestre
left a comment
sylvestre
left a comment
There was a problem hiding this comment.
it regresses all benchmarks
Implement dynamic pipeline depth to allow more overlapping between reading, sorting, and writing operations. The `pipeline_depth` setting now controls the number of in-flight chunks, with a minimum of 2, improving performance for large datasets by reducing I/O bottlenecks.
|
GNU testsuite comparison: |
…U sort Adjust the compare_by function to swap the order of byte comparison (b.line vs a.line) specifically in SortMode::Default, ensuring raw byte sorting behavior aligns with GNU sort. This fixes inconsistencies in default sorting order for raw bytes, improving compatibility with expected POSIX behavior.
- Swapped lexicographic comparison from a.line.cmp(b.line) to b.line.cmp(a.line) to correct ordering behavior in compare_by function. - Simplified SortMode::Default from custom_str_cmp with options to plain b_str.cmp(a_str), assuming default sort now uses standard string comparison without special handling for non-printing, dictionary, or case sensitivity. This ensures consistent and correct sorting behavior in the sort utility.
When both lines are valid UTF-8, compare them lexicographically for proper text sorting; otherwise fall back to reversed byte comparison to match GNU sort behavior for raw bytes. Improves sorting accuracy for text data while preserving byte-level sorting for binary input.
- Move fast lexicographic comparison to early check and remove duplication - Replace simple reverse comparison in SortMode::Default with custom_str_cmp function - Ensures proper handling of ignore_non_printing, dictionary_order, and ignore_case options
- Added token_buffer Vec<Field> to ChunkContents and RecycledChunk - Added utf8_cache Vec<Option<&'a str>> to LineData and RecycledChunk - Updated recycle, read, and parse_lines handling for new fields - Enables efficient tokenization and UTF-8 caching in sort operations
Use clamp() instead of chained max().min() for better readability. This replaces explicit bounds checking in merge_chunk_capacity() with the more idiomatic clamp method, achieving the same effect while reducing code verbosity.
Implement dynamic pipeline depth to allow more overlapping between reading, sorting, and writing operations. The `pipeline_depth` setting now controls the number of in-flight chunks, with a minimum of 2, improving performance for large datasets by reducing I/O bottlenecks.
|
GNU testsuite comparison: |
Add a new `filtered_lines` field to `LineData` for caching processed line versions. Introduce `build_filtered_line` function to filter out non-printing or non-dictionary characters and handle case-ignore, enabling new sorting options. Update chunk recycling and parsing to manage the new field. Add buffer size normalization and pipeline depth tuning for improved memory efficiency in external sorting.
Introduce `ReaderWriterConfig` struct to bundle `buffer_size` and `pipeline_depth` parameters, reducing function signature complexity and improving code readability in external sorting logic. Replace separate parameters with the config reference in `ext_sort`, `reader_writer`, and `read_write_loop` functions. Minor efficiency tweak in `tuned_pipeline_depth` using `.clamp()` for bounds checking.
|
GNU testsuite comparison: |
Contributor
Author
|
I'm making changes to this pull request for performance tuning, but regressions always occur. |
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
compare_byso the ASCII fast path is shared and still respects--reverseSortMode::Defaultthroughcustom_str_cmpto ensure--ignore-non-printing,--dictionary-order, and--ignore-caseremain effective even when the fast path triggersfix
#9264