Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rustc: allow non-empty ParamEnv's in global trait select/eval caches. #66821

Merged
merged 2 commits into from
Dec 11, 2019
Merged

rustc: allow non-empty ParamEnv's in global trait select/eval caches. #66821

merged 2 commits into from
Dec 11, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Nov 27, 2019 •
edited
Loading

Based on #66963

This appears to alleviate the symptoms of #65510 locally (without fixing WF directly), and is potentially easier to validate as sound (since it's a more ad-hoc version of queries we already have).

I'm opening this PR primarily to test the effects on perf.

r? @nikomatsakis cc @rust-lang/wg-traits

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2019
@eddyb
Copy link
Member Author

eddyb commented Nov 27, 2019

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 27, 2019

⌛ Trying commit d637c85 with merge 06db465248f3aa76f16924e778609dec610063a4...

@eddyb eddyb mentioned this pull request Nov 27, 2019
Open
@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Nov 27, 2019

☀️ Try build successful - checks-azure
Build commit: 06db465248f3aa76f16924e778609dec610063a4 (06db465248f3aa76f16924e778609dec610063a4)

@rust-timer
Copy link
Collaborator

Queued 06db465248f3aa76f16924e778609dec610063a4 with parent 04e69e4, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 06db465248f3aa76f16924e778609dec610063a4, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Nov 28, 2019

Looks like the wins aren't as significant as #66020 (comment), but they are comparable.

bors added a commit that referenced this pull request Nov 28, 2019
@bors
Auto merge of #66838 - eddyb:wf-skip-plus-global-caching, r=<try>
685e48b 
[WIP] [DO NOT MERGE] combine #66020 and #66821.

That is, the two fixes for #65510, and only for perf testing purposes.
The fact that they both work to a comparable extent, while touching different parts of the trait system, made me curious if there would be any gains from having both.

r? @nikomatsakis
@eddyb
Copy link
Member Author

eddyb commented Nov 28, 2019

Oh, also, the max-rss for the above run has increased across the board (because we're caching more), but it's not as bad as I was worried it might be.

Then again, looking at #66020's own max-rss, I'd argue it's noisy, because that PR only removes work, so I don't see why it could/would increase memory usage.

@eddyb eddyb mentioned this pull request Nov 28, 2019
Closed
@eddyb eddyb force-pushed the global-trait-caching branch 2 times, most recently from 8f1af1d to 36b23ed Compare December 2, 2019 15:44
@eddyb eddyb mentioned this pull request Dec 2, 2019
Merged
@eddyb eddyb force-pushed the global-trait-caching branch from 36b23ed to a266ea0 Compare December 2, 2019 16:54
@eddyb eddyb changed the title [WIP] rustc: experiment with ParamEnv-aware caching in traits::select. rustc: allow non-empty ParamEnv's in global trait select/eval caches. Dec 2, 2019
@eddyb
Copy link
Member Author

eddyb commented Dec 2, 2019

Take 2 (after getting tests to pass):
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 2, 2019

⌛ Trying commit a266ea0 with merge 1ff0441...

bors added a commit that referenced this pull request Dec 2, 2019
@bors
Auto merge of #66821 - eddyb:global-trait-caching, r=<try>
1ff0441 
rustc: allow non-empty ParamEnv's in global trait select/eval caches.

*Based on #66963*

This appears to alleviate the symptoms of #65510 locally (without fixing WF directly), and is potentially easier to validate as sound (since it's a more ad-hoc version of queries we already have).

I'm opening this PR primarily to test the effects on perf.

r? @nikomatsakis cc @rust-lang/wg-traits
@bors
Copy link
Contributor

bors commented Dec 2, 2019

☀️ Try build successful - checks-azure
Build commit: 1ff0441 (1ff04410af642dde1480ae29b085544c2d05c33c)

@rust-timer
Copy link
Collaborator

Queued 1ff0441 with parent 2da942f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 1ff0441, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Dec 3, 2019

Looks comparable, so the changes I had to make didn't cause much of a difference.

@Dylan-DPC-zz
Copy link

Blocked on #66963

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2019
@nikomatsakis
Copy link
Contributor

So I left a comment on #66963 explaining that I have some mild trepidation about the behavior of projection when inference variables are involved, but I'm inclined to land this PR since I don't think it will do anything incorrect.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 10, 2019

📌 Commit a266ea0 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Dec 10, 2019
@eddyb
Copy link
Member Author

eddyb commented Dec 10, 2019

@bors rollup=never

@bors
Copy link
Contributor

bors commented Dec 11, 2019

⌛ Testing commit a266ea0 with merge 90b957a...

bors added a commit that referenced this pull request Dec 11, 2019
@bors
Auto merge of #66821 - eddyb:global-trait-caching, r=nikomatsakis
90b957a 
rustc: allow non-empty ParamEnv's in global trait select/eval caches.

*Based on #66963*

This appears to alleviate the symptoms of #65510 locally (without fixing WF directly), and is potentially easier to validate as sound (since it's a more ad-hoc version of queries we already have).

I'm opening this PR primarily to test the effects on perf.

r? @nikomatsakis cc @rust-lang/wg-traits
RalfJung
RalfJung reviewed Dec 11, 2019
View reviewed changes
src/test/ui/type-alias-impl-trait/bound_reduction2.stderr
Comment on lines +7 to +8
LL | fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> {
| -- help: consider further restricting this bound: `T: TraitWithAssoc +`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error looks pretty wrong though, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but it makes sense in the context.
It's the error you get if the other errors don't prevent the impl Trait from getting the incompatible concrete type, because of the type parameter that is now in the wrong ParamEnv, which doesn't have the original bound.

This is pre-existing, it was just hidden by the bug #66963 fixes.

@bors
Copy link
Contributor

bors commented Dec 11, 2019

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 90b957a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2019
@bors bors merged commit a266ea0 into rust-lang:master Dec 11, 2019
This was referenced Dec 11, 2019
Merged
Merged
@eddyb eddyb deleted the global-trait-caching branch December 11, 2019 17:10
@eddyb eddyb mentioned this pull request Dec 12, 2019
Closed
@Aaron1011 Aaron1011 mentioned this pull request Jan 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eddyb @rust-timer @bors @rust-highfive @Dylan-DPC-zz @nikomatsakis @RalfJung