Make TypeFolder::fold_* return Result
#91230
Merged
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
2 tasks
|
@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 362320b3e16971f16544d13786bd4f6fcfb7465f with merge 045cf79edfbfad98d41a0fefbb824ac4306f2dbd... |
|
☀️ Try build successful - checks-actions |
|
Queued 045cf79edfbfad98d41a0fefbb824ac4306f2dbd with parent 862962b, future comparison URL. |
camelid
added
the
T-compiler
|
Finished benchmarking commit (045cf79edfbfad98d41a0fefbb824ac4306f2dbd): comparison url. Summary: This change led to large relevant mixed results 🤷 in compiler performance.
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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
|
Perf is better than last time, but not neutral. It would be nice to look into it a bit, but I also think this is "landable" as-is. |
|
(I still have to do an actual review) |
Co-authored-by: Alan Egerton <eggyal@gmail.com>
Co-authored-by: Alan Egerton <eggyal@gmail.com>
Co-authored-by: Alan Egerton <eggyal@gmail.com>
Co-authored-by: Alan Egerton <eggyal@gmail.com>
eggyal
force-pushed
the
fallible-type-fold
branch
from
362320b to
6db9605
Compare
cachegrind diff for the worst scenario (diesel full check)I haven't yet made sense of this result, but from a first glance I'm wondering if maybe a cache is no longer being used to the same degree. For example, the early/short-circuiting return in the event of error that is introduced just before this cache insertion: rust/compiler/rustc_trait_selection/src/traits/query/normalize.rs Lines 326 to 327 in 6db9605 That said, I'm not convinced this is the actual source of the regression, so I'll keep digging... unless anything obvious jumps out at a more seasoned mind in the interim? |
|
Per discussion on Zulip the perf regression here is suspected to be due LLVM's PGO decisions. @rustbot label: +perf-regression-triaged Marking ready for review. |
rustbot
added
the
perf-regression-triaged
jackh726
requested changes
Nov 26, 2021
There was a problem hiding this comment.
A couple comments, but mostly good.
One thought I had while reading this is it does introduce quite a bit of boilerplate for the few fallible cases. I wonder if there is some way to encode this into the trait to make it nicer? That would be in a followup regardless.
eggyal
force-pushed
the
fallible-type-fold
branch
from
ae98c5e to
04f1c09
Compare
I pushed 9f714ef, which delegates from In fact, I wonder whether |
|
I do mean altering the TypeFolder api, yes. But definitely for followup, if at all. AFAIK, rustc_data_structures has no stability requirements. But removing map_id could be done in a followup PR. This LGTM. @bors r+ |
|
📌 Commit afa6f92 has been approved by |
bors
added
S-waiting-on-bors
|
Thanks for picking this up @eggyal! |
|
☀️ Test successful - checks-actions |
This was referenced Nov 28, 2021
|
Finished benchmarking commit (e6d2de9): comparison url. Summary: This change led to large relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 2, 2021
…llible-folders, r=jackh726 Reduce boilerplate around infallible folders Further to rust-lang#91230 (comment) r? `@jackh726`
|
It's curious how the post-merge perf result is so different to the pre-merge result at #91230 (comment) (that was put down to LLVM/PGO optimisations). I don't think anything material changed in this PR between the two runs, so I guess something material was merged in the interim. Nevertheless: cachegrind diff for the worst scenario (deep-vector incr-unchanged opt)Some material differences there in hashing, perhaps indicating a difference in cache use—possibly around resolving consts? Because I don't think this is due to PGO: |
|
Error: Label because can only be set by Rust team members Please let |
rustbot
removed
the
perf-regression-triaged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implements rust-lang/compiler-team#432.
Initially this is just a rebase of @LeSeulArtichaut's work in #85469 (abandoned; see #85485 (comment)). At that time, it caused a regression in performance that required some further exploration... with this rebased PR bors can hopefully report some perf analysis from which we can investigate further (if the regression is indeed still present).
r? @jackh726 cc @nikomatsakis