uucore: make Range::merge linear instead of quadratic - #14359
haydonryan wants to merge 2 commits into
Conversation
|
GNU testsuite comparison: |
|
any reason why codspeed don't detect the improvements? |
|
The dedup is Indeed better, as you don't shift the queue every time You're doing a sort which is n*logarithmic to begin with. @sylvestre TLDR : the code is still better, but not as much as the clanker said |
|
A lot of things going on at the moment. Will get onto this this weekend. Apologies for the delay |
0abdb08 to
df42a7e
Compare
|
Again, apologies for the slow replies - I have an ongoing family medical situation that is taking a lot of my focus.
I had to test using a custom harness, as cut is the tool that exposes this but the rest of the tool dominated the testing. Not sure how codspeed works, if it's just using a test suite on the individual functions or what. @anastygnome Your code suggestion was excellent! This is the first time i've had a PR have a code review suggestion - please let me know if I did it correctly (I also wasn't sure if I should give you attribution or how to do that on the updated line). Ran the clanker:
Runtime (
|
| Case | n | baseline sort() |
variant by_key |
Speedup |
|---|---|---|---|---|
| random-overlap | 100,000 | 8.19 | 2.39 | 3.4× |
| random-overlap | 300,000 | 21.63 | 7.82 | 2.8× |
| random-overlap | 1,000,000 | 77.47 | 27.15 | 2.9× |
| all-overlap | 1,000,000 | 2.28 | 1.04 | 2.2× |
| disjoint | 1,000,000 | 2.22 | 1.06 | 2.1× |
Measured with an isolated mergebench harness (uucore [[example]]) calling
Range::merge directly on synthetic range lists — no process startup, argv,
parse, or I/O noise (same approach as the original coll2.md A/B).
|
@haydonryan thanks! If you want to add a contribution, amend your commit message, add a blank line after the title, then on the third line you can put
or anyone really :) |
|
Thanks! Added! |
Co-authored-by: anastygnome <15268361+anastygnome@users.noreply.github.com>
Deepseek found another optimization.
This one converts the loop from O(n^2) to O(n) for ranges.
I ran a few options for this PR, including one that was smaller in size by 96 bytes, but this is faster overall.
LLM generated below here:
Range::merge(src/uucore/src/lib/features/ranges.rs) merged overlappingranges with
ranges.remove(j)inside awhileloop. BecauseVec::removeshifts the tail, heavily-overlapping range lists were O(n²). Only caller is
cut(viaRange::from_list).Replaced with a single
Vec::dedup_bypass that extends the bucket'shighon overlap. Output is unchanged — still sorted, disjoint, and adjacent ranges
are not merged. The only subtlety is that
dedup_by(a, b)passesa= newelement,
b= bucket, so the closure must extendb.high(the kept element),not
a.high(the dropped one).Measurement (same harness, release)
Both versions measured in one harness, same input, realistic overlapping ranges:
The O(n) pass is dramatically faster at 30k+ ranges, but real
cutinvocationsuse a handful of fields (argv caps ~30k, and a typical
-flist is 1-100), sothe merge is sub-microsecond, once-at-startup work there. The value of this
change is not a user-visible speedup: it removes the O(n²) blowup (a large
overlapping field list would otherwise hang
cut) and shrinks the binary by256 B.
dedup_bywas also the smallest/fastest of the four O(n) variants A/B'd(baseline, extra-
Vec, in-placeswap,dedup_by).dedup_bywas also the fastest of the four O(n) variants A/B'd (baseline,extra-
Vec, in-placeswap,dedup_by) in every all-overlap case; the inplace variants are not measurably faster and compile to a larger binary.
Verification
cargo fmtclean,cargo clippy --release --bin coreutilsclean (0 warnings)