-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lift TypeFoldable over Result and remove the Relate hack in combine/nll #58333
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #58389) made this pull request unmergeable. Please resolve the merge conflicts. |
I'll check that this doesn't hurt perf @bors try |
⌛ Trying commit 7abc7ed7193d23d2be7898a6d94d282a83687b77 with merge 258c318bc56c15d4c0ac14f669a6b81f097bf17e... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
src/librustc/ty/fold.rs
Outdated
#[inline] | ||
/// If `false` - the default - then `ty::Invariant` might be used instead of the | ||
/// correct variance when folding an item with a variance. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: blank line in the middle of a ///
comment? Seems unusual
|
||
/// Otherwise, the correct variance is looked up from the tcx, which can | ||
/// be a performance and cycle hazard. | ||
fn use_variances(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok, but also error-prone. i.e., if somebody forgets to make use_variances
be true, we'll have a problem. It might be worth thinking if we can turn that into a dynamic assertion or something. But I don't obviously see how to do that with this setup as in this PR -- I was imagining that if the variance was tracked in a field, then accessing it could assert that use_variances
is true or something...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make variances
an Option
, but t hen the users would need to handle that. I think that forgetting to use use_variances
would cause fairly visible errors, however, so I don't think this is a large risk.
fn a_is_expected(&self) -> bool { | ||
true | ||
fn use_variances(&self) -> bool { | ||
if self.ambient_variance == ty::Variance::Invariant { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in the branches that I was working on, I made the folder track the ambient variance iirc. I don't however remember exactly how I did this =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a thing that Rust handles particularly well
☀️ Test successful - checks-travis |
@rust-timer build 258c318 |
Insufficient permissions to issue commands to rust-timer. |
Could someone with perfbot permissions give me a rust-timer build? |
@rust-timer build 258c318bc56c15d4c0ac14f669a6b81f097bf17e |
Success: Queued 258c318bc56c15d4c0ac14f669a6b81f097bf17e with parent e544947, comparison URL. |
I'm poking with the fairly hot |
@bors try |
Lift TypeFoldable over Result and remove the Relate hack in combine/nll No functional changes intended - just a cleanup. r? @nikomatsakis
Bad github, deleting my test results like that, now I need another try |
Finished benchmarking try commit 258c318bc56c15d4c0ac14f669a6b81f097bf17e |
Serde is very red in this PR (24%!). Need to investigate why. |
☀️ Test successful - checks-travis |
@rust-timer build c5e0b61 |
Success: Queued c5e0b61 with parent eac0908, comparison URL. |
Finished benchmarking try commit c5e0b61 |
Ping from triage, @arielb1 seems like the benchmark results are in? |
Yea, seems like a big perf problem. I think I know what causes it, but I should try to figure it out the moment I have a bit more time. |
⌛ Trying commit e2e437e99bedd41bddc9d9e9fd3d8be2e1310f2f with merge e651546711e886f53b1d0380fbda11449dcc269a... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors try |
Lift TypeFoldable over Result and remove the Relate hack in combine/nll No functional changes intended - just a cleanup. r? @nikomatsakis
☀️ Try build successful - checks-travis |
Could someone do a new perf run? |
@rust-timer build 413f247 You should also have the permissions to queue perf builds as well. |
Success: Queued 413f247 with parent c0086b9, comparison URL. |
Finished benchmarking try commit 413f247 |
Perf is now somehow green? |
@arielb1 seems good? :) maybe rebase and repeat if you are nervous? |
ping from triage @arielb1 you need to rebase |
Ping from triage, @arielb1 still need to rebase ;) |
ping from triage @arielb1 |
No functional changes intended - just a cleanup.
r? @nikomatsakis