Optimize ChunkedBitSet dense relations #94625
Conversation
rustbot
added
the
T-compiler
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nnethercote (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
Let's do a perf run on CI before I review this. @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 2ffe6f3 with merge 7d921874adb595b7bb13072cc2c7b70a015e679d... |
|
☀️ Try build successful - checks-actions |
|
Queued 7d921874adb595b7bb13072cc2c7b70a015e679d with parent 9fcbc32, future comparison URL. |
|
Finished benchmarking commit (7d921874adb595b7bb13072cc2c7b70a015e679d): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
| assert_eq!(self.domain_size, other.domain_size()); | ||
|
|
||
| let mut changed = false; | ||
| for (chunk, other_words) in self.chunks.iter_mut().zip(other.words().chunks(CHUNK_WORDS)) { |
There was a problem hiding this comment.
Where is this chunks() method defined?
There was a problem hiding this comment.
|
Thank you for the pull request! Well done on navigating the contribution process and completing it, everything looks fine in terms of how you've gone about it. I particularly like that you split it up into three logical commits. Hopefully we can look forward to more contributions from you in the future 😄 I admit I am torn on this one. On one hand, the code looks good, much how I would expect. On the other hand, it's a bunch of extra lines of code and corresponding complexity that doesn't provide any measurable performance benefit. I have written plenty of PRs that I thought would help performance and then didn't, and ended up closing them, and my sense of engineering trade-offs is pushing me to do the same thing here. Sorry about that. One thing you could do is file another PR that adds comments to |
Don't be! I just needed something to work on at a hack night to improve my rust skills, no reason to merge unless it adds value.
Sure I can do that :) |
…nethercote Add link to closed PR for future optimizers of ChunkedBitSet relations While optimizing these operations proved unfruitful w.r.t. improving compiler performance right now, faster versions might be needed at a later time. This PR adds a link in the FIXME to save any future optimizers some time, as requested by `@nnethercote` in rust-lang#94625. r? `@nnethercote`
ChunkedBitSet::subtract(&HybridBitSet)showed up in a callgrind profile grabbed of thehelloworldbenchmark while playing around with rustc-perf, and the FIXMEs left behind there by @nnethercote pointed to a way to optimize it.The resulting improvements on the
rustc-perfbenchmarks are meager to say the least, but perhaps the changes are of interest anyway.I'm unacquainted with the compiler code and the tooling in general so I apologize in advance for any missteps!
PS: Sorry for the double PR, the first one was supposed to be a draft!
r? @nnethercote