[perf] Add explicit Iterator::count impl for ChunkBy.#158866
Conversation
rustbot
added
S-waiting-on-author
|
cc @Kobzol for perf run |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
[perf] Add explicit `Iterator::count` impl for `ChunkBy`.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (546c6e5): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.7%, secondary -5.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.9%, secondary -3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 491.018s -> 490.286s (-0.15%) |
rustbot
removed
the
S-waiting-on-perf
|
It's interesting that we see regressions at all. I expected a ~zero impact finding due to |
|
Those two benchmarks are essentially just hello world, it might just be noise, or the stdlib becoming a tiny bit larger to load or something. A change like this most likely won't be caught by our compile benchmark suite (which doesn't mean it's not worth landing). |
|
This looks fine to me, and probably performs better than the default. r=me if you want to... |
|
@bors delegate |
|
✌️ @obi1kenobi, you can now approve this pull request! If @Kobzol told you to " |
rustbot
added
S-waiting-on-review
|
I updated the PR description with the results of a benchmark using a real-world library's public API, plus a microbenchmark extracted from that use site. Both agree there's a material win here, and seeing vectorization start happening here is the cherry on top. |
|
@bors r=joboet |
rust-bors
Bot
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - #155429 (Support `u128`/`i128` c-variadic arguments) - #158899 (Fix `unaligned_volatile_store` by removing `MemFlags::UNALIGNED`) - #158640 (Handle nested inline consts in const argument checks) - #158866 ([perf] Add explicit `Iterator::count` impl for `ChunkBy`.) - #158912 (Introduce new bootstrap config section for PGO configuration) - #159040 (Print step stack trace when bootstrap panics) - #159056 (Print a loud error if the configured LLDB cannot be found)
Rollup merge of #158866 - obi1kenobi:pg/iterator-chunk-by, r=joboet [perf] Add explicit `Iterator::count` impl for `ChunkBy`. Prior to this PR, `.chunk_by(..).count()` constructs were using the default `Iterator::count` implementation for `ChunkBy`, which was based on `Iterator::next()`. We can do noticeably better by writing a dedicated implementation, as measured [at this real-world call site in `omnibor-rs`](https://github.com/omnibor/omnibor-rs/blob/116c5c2281219dece9da4eddc7d2e2c47e4c81c4/omnibor/src/gitoid/internal.rs#L186). A local benchmark on my macOS laptop shows that using `omnibor-rs`'s public API to invoke `ArtifactId::sha256()` in a tight loop, with appropriate black-boxing, experiences a 5-7% CPU time win with the dedicated implementation. Of course the benchmark is sensitive to the exact length and contents of the file, hence the 5-7% range. But the 95% confidence interval for the amount of improvement in every benchmark I ran is <1% wide, so I am quite confident this is an improvement. I also extracted the relevant `omnibor-rs` use of `.chunk_by(..).count()` into a microbenchmark, which shows a 2.1-2.4x throughput improvement (-51.5-57.7% CPU time) with the new implementation. Disassembly of the baseline vs new implementation shows that the optimizer was able to vectorize the new implementation, while the baseline was not vectorized. **AI disclosure:** The optimization opportunity here was discovered as part of a systematic probe for missed optimizations utilizing both traditional and AI tools. The code here was initially prototyped and vetted by AI tools, followed by additional manual work. I secured approval in advance from the reviewer I pinged. I stand behind the quality of the code I'm submitting, and I vouch it's as good or better compared to if I had written every line by my own hand.
|
Hey Predrag 😄! Could you make sure to include the LLM-disclosure in the description of draft PRs as well? I don't think it would have changed anything about my approval of this PR, but it feels a bit nicer not to be surprised by such a note when revisiting things. Thanks, and no worries! |
|
Apologies for the unexpected surprise! To be honest I didn't expect anyone to review a draft PR with no description 😅 I'll try to indicate going forward whether a PR is in a state I consider reviewable or just for measurement purposes only. As you know for perf work it's often useful to know whether a change is perf-positive at all before polishing it, and as you see my AI note includes a strong endorsement of the code quality which wouldn't necessarily apply on a perf-measurement-only PR. I might have to come up with a different AI note for such situations. I'll try to figure out something! |
6 participants
Prior to this PR,
.chunk_by(..).count()constructs were using the defaultIterator::countimplementation forChunkBy, which was based onIterator::next().We can do noticeably better by writing a dedicated implementation, as measured at this real-world call site in
omnibor-rs.A local benchmark on my macOS laptop shows that using
omnibor-rs's public API to invokeArtifactId::sha256()in a tight loop, with appropriate black-boxing, experiences a 5-7% CPU time win with the dedicated implementation. Of course the benchmark is sensitive to the exact length and contents of the file, hence the 5-7% range. But the 95% confidence interval for the amount of improvement in every benchmark I ran is <1% wide, so I am quite confident this is an improvement.I also extracted the relevant
omnibor-rsuse of.chunk_by(..).count()into a microbenchmark, which shows a 2.1-2.4x throughput improvement (-51.5-57.7% CPU time) with the new implementation. Disassembly of the baseline vs new implementation shows that the optimizer was able to vectorize the new implementation, while the baseline was not vectorized.AI disclosure: The optimization opportunity here was discovered as part of a systematic probe for missed optimizations utilizing both traditional and AI tools. The code here was initially prototyped and vetted by AI tools, followed by additional manual work. I secured approval in advance from the reviewer I pinged. I stand behind the quality of the code I'm submitting, and I vouch it's as good or better compared to if I had written every line by my own hand.