-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
convert NLL ops to caches #51538
convert NLL ops to caches #51538
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@bors try |
⌛ Trying commit 7aa36ed446038319115b1a3cf70cf67ef221dc92 with merge adbbd12ea685afa27f98c032745585bb98f42e81... |
cc @Mark-Simulacrum — if you get a chance, once the try run is complete, can you off a perf run? (I will try to ping you again when that has happened...) |
I've queued the perf build, I think it'll start late enough to be in time for try to complete. |
☀️ Test successful - status-travis |
Results show a decent win, though not (yet!) where I hope to get it:
|
r? @pnkfelix |
40e3e02
to
d3115ab
Compare
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 |
30b3ad8
to
9e67309
Compare
@bors try |
[WIP] Convert NLL ops to caches This is a extension of <#51460>. It uses a lot more caching than we used to do. This caching is not yet as efficient as it could be, but I'm curious to see the current perf results.
This PR is -- I believe -- at a pretty decent state. |
(Once the try build completes, I would like to get a perf run.) |
src/librustc_traits/type_op.rs
Outdated
let ProvePredicate { | ||
param_env, | ||
predicate, | ||
} = key; |
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 is written differently than subtype/eq (i.e. not unpacking in closure args), why?.
src/librustc_traits/type_op.rs
Outdated
ObligationCause::dummy(), | ||
param_env, | ||
predicate, | ||
)], |
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.
Interesting, I would've expected you to pass fulfill_cx
to the closure instead.
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.
Oookay turns out GitHub deleted replies to comments... I was only wanting to move it.
@nikomatsakis replied to the original version of this (found in my email):
Hmm. That's not a bad idea. I've been trying to encapsulate the FulfillmentContext to some degree, since it's part of the "old trait system" and not really a part of the new one, and for most of the methods they have an InferOk all setup to return. But the inefficiency here does annoy me so....
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 |
7140559
to
b181195
Compare
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 |
c37de00
to
f48efa9
Compare
r=me with #51538 (comment) (passing |
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 |
f48efa9
to
56ccfc4
Compare
56ccfc4
to
1523de3
Compare
@bors r=eddyb |
📌 Commit 1523de3 has been approved by |
@bors p=1 kind of critical for NLL |
convert NLL ops to caches This is a extension of <#51460>. It uses a lot more caching than we used to do. This caching is not yet as efficient as it could be, but I'm curious to see the current perf results. This is the high-level idea: in the MIR type checker, use [canonicalized queries](https://rust-lang-nursery.github.io/rustc-guide/traits/canonical-queries.html) for all the major operations. This is helpful because the MIR type check is operating in a context where all types are fully known (mostly, anyway) but regions are completely renumbered. This means we often wind up with duplicate queries like `Foo<'1, '2> :Bar` and `Foo<'3, '4>: Bar`. Canonicalized queries let us re-use the results. By the final commit in this PR, we can essentially just "read off" the resulting region relations and add them to the NLL type check.
☀️ Test successful - status-appveyor, status-travis |
This is a extension of #51460. It uses a lot more caching than we used to do. This caching is not yet as efficient as it could be, but I'm curious to see the current perf results.
This is the high-level idea: in the MIR type checker, use canonicalized queries for all the major operations. This is helpful because the MIR type check is operating in a context where all types are fully known (mostly, anyway) but regions are completely renumbered. This means we often wind up with duplicate queries like
Foo<'1, '2> :Bar
andFoo<'3, '4>: Bar
. Canonicalized queries let us re-use the results. By the final commit in this PR, we can essentially just "read off" the resulting region relations and add them to the NLL type check.