perf: Revert accidental inclusion of a part of #69218#71996
perf: Revert accidental inclusion of a part of #69218#71996bors merged 2 commits intorust-lang:masterfrom
Conversation
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This was accidentally included in rust-lang#69494 after a rebase and given how much `inflate` and `keccak` stresses the obligation forest seems like a likely culprit to the regression in those benchmarks. (It is necessary in rust-lang#69218 as obligation forest needs to accurately track the root variables or unifications will get lost)
|
That seems like the wrong PR |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 1d8489c with merge b80dbeb9b6dc6b59230376c05f5d2dd14a757f54... |
|
☀️ Try build successful - checks-azure |
|
Queued b80dbeb9b6dc6b59230376c05f5d2dd14a757f54 with parent a08c473, future comparison URL. |
|
@bors r+ the perf run results, however, aren't visible yet |
|
📌 Commit 1d8489c has been approved by |
bors
added
S-waiting-on-bors
|
@bors delegate+ |
|
✌️ @Marwes can now approve this pull request |
|
Finished benchmarking try commit b80dbeb9b6dc6b59230376c05f5d2dd14a757f54, comparison URL. |
|
@bors rollup=never (please do that for PRs that are suspected to impact perf) |
|
@bors r- Hmm |
bors
added
S-waiting-on-author
|
@Marwes doesn't seem to have much impact on keccak... any thoughts? |
|
Trying to figure that out but I really can't find another place which would disproportionately affect keccak and inflate :S |
Reclaims most of the regression in inflate
|
Adding some @nnethercote I tried cachegrind but for whatever reason I can't get it to work. Tried with callgrind also and tried to view the output in kcachegrind (which I have used before) but I just get addresses displayed. I guess something makes valgrind/cachegrind/ etc not understand the debug information. (perf + hotspot works fine though, as usual :/) |
|
@Marwes: your comment made me realize that the docs are missing a crucial part about setting a |
|
Used |
|
Didn't work how? Can you show me some example output? |
|
Running And then running (Diffs a file with itself so everything is zero, still there are not rust functions in the diff)
|
|
Note that I'm not sure we'll rebuild the compiler for you, so you may need to x.py clean and rebuild. Also, debuginfo level of 1 should be sufficient I suspect and significantly lighter on disk usage - I've personally seen IIRC tens of gigabytes of space savings. |
|
I also recommend running Cachegrind via |
|
Cleaning and rebuilding does not help, nor does just using level 1 or running via collector. Could use a perf run to validate that inlining helps. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit ebc7eda with merge ad74291237c3c0233ace59eec9db124ff1c17082... |
|
☀️ Try build successful - checks-azure |
|
Queued ad74291237c3c0233ace59eec9db124ff1c17082 with parent aeca4d6, future comparison URL. |
|
Finished benchmarking try commit ad74291237c3c0233ace59eec9db124ff1c17082, comparison URL. |
|
Great work @Marwes! |
|
It doesn't recover all the performance but it is at least most of it (1.5% regression remaining locally). However with #69218 it probably doesn't matter anyway, since that improves keccak and inflate by so much that it won't really be noticed. |
|
@bors r+ rollup=neve |
|
📌 Commit ebc7eda has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup=never |
|
☀️ Test successful - checks-azure |
|
The perf improvements from the landing are here. Nice work! |
11 participants
This was accidentally included in #69464 after a rebase and given
how much
inflateandkeccakstresses the obligation forest seemslike a likely culprit to the regression in those benchmarks.
(It is necessary in #69218 as obligation forest needs to accurately
track the root variables or unifications will get lost)