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.

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

Properly track DepNodes in trait evaluation provisional cache #93064

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

Aaron1011
Copy link
Member

Fixes #92987

During evaluation of an auto trait predicate, we may encounter a cycle.
This causes us to store the evaluation result in a special 'provisional
cache;. If we later end up determining that the type can legitimately
implement the auto trait despite the cycle, we remove the entry from
the provisional cache, and insert it into the evaluation cache.

Additionally, trait evaluation creates a special anonymous DepNode.
All queries invoked during the predicate evaluation are added as
outoging dependency edges from the DepNode. This DepNode is then
store in the evaluation cache - if a different query ends up reading
from the cache entry, it will also perform a read of the stored
DepNode. As a result, the cached evaluation will still end up
(transitively) incurring all of the same dependencies that it would
if it actually performed the uncached evaluation (e.g. a call to
type_of to determine constituent types).

Previously, we did not correctly handle the interaction between the
provisional cache and the created DepNode. Storing an evaluation
result in the provisional cache would cause us to lose the DepNode
created during the evaluation. If we later moved the entry from the
provisional cache to the evaluation cache, we would use the DepNode
associated with the evaluation that caused us to 'complete' the cycle,
not the evaluatoon where we first discovered the cycle. As a result,
future reads from the evaluation cache would miss some incremental
compilation dependencies that would have otherwise been added if the
evaluation was not cached.

Under the right circumstances, this could lead to us trying to force
a query with a no-longer-existing DefPathHash, since we were missing
the (red) dependency edge that would have caused us to bail out before
attempting forcing.

This commit makes the provisional cache store the DepNode create
during the provisional evaluation. When we move an entry from the
provisional cache to the evaluation cache, we create a new DepNode
that has dependencies going to both of the evaluation DepNodes we
have available. This ensures that cached reads will incur all of
the necessary dependency edges.

Fixes rust-lang#92987

During evaluation of an auto trait predicate, we may encounter a cycle.
This causes us to store the evaluation result in a special 'provisional
cache;. If we later end up determining that the type can legitimately
implement the auto trait despite the cycle, we remove the entry from
the provisional cache, and insert it into the evaluation cache.

Additionally, trait evaluation creates a special anonymous `DepNode`.
All queries invoked during the predicate evaluation are added as
outoging dependency edges from the `DepNode`. This `DepNode` is then
store in the evaluation cache - if a different query ends up reading
from the cache entry, it will also perform a read of the stored
`DepNode`. As a result, the cached evaluation will still end up
(transitively) incurring all of the same dependencies that it would
if it actually performed the uncached evaluation (e.g. a call to
`type_of` to determine constituent types).

Previously, we did not correctly handle the interaction between the
provisional cache and the created `DepNode`. Storing an evaluation
result in the provisional cache would cause us to lose the `DepNode`
created during the evaluation. If we later moved the entry from the
provisional cache to the evaluation cache, we would use the `DepNode`
associated with the evaluation that caused us to 'complete' the cycle,
not the evaluatoon where we first discovered the cycle. As a result,
future reads from the evaluation cache would miss some incremental
compilation dependencies that would have otherwise been added if the
evaluation was *not* cached.

Under the right circumstances, this could lead to us trying to force
a query with a no-longer-existing `DefPathHash`, since we were missing
the (red) dependency edge that would have caused us to bail out before
attempting forcing.

This commit makes the provisional cache store the `DepNode` create
during the provisional evaluation. When we move an entry from the
provisional cache to the evaluation cache, we create a *new* `DepNode`
that has dependencies going to *both* of the evaluation `DepNodes` we
have available. This ensures that cached reads will incur all of
the necessary dependency edges.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 19, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2022
@michaelwoerister
Copy link
Member

Thanks, @Aaron1011! The fix looks valid to me. Great comments!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 24, 2022

📌 Commit 02f1a56 has been approved by michaelwoerister

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2022
…ichaelwoerister

Properly track `DepNode`s in trait evaluation provisional cache

Fixes rust-lang#92987

During evaluation of an auto trait predicate, we may encounter a cycle.
This causes us to store the evaluation result in a special 'provisional
cache;. If we later end up determining that the type can legitimately
implement the auto trait despite the cycle, we remove the entry from
the provisional cache, and insert it into the evaluation cache.

Additionally, trait evaluation creates a special anonymous `DepNode`.
All queries invoked during the predicate evaluation are added as
outoging dependency edges from the `DepNode`. This `DepNode` is then
store in the evaluation cache - if a different query ends up reading
from the cache entry, it will also perform a read of the stored
`DepNode`. As a result, the cached evaluation will still end up
(transitively) incurring all of the same dependencies that it would
if it actually performed the uncached evaluation (e.g. a call to
`type_of` to determine constituent types).

Previously, we did not correctly handle the interaction between the
provisional cache and the created `DepNode`. Storing an evaluation
result in the provisional cache would cause us to lose the `DepNode`
created during the evaluation. If we later moved the entry from the
provisional cache to the evaluation cache, we would use the `DepNode`
associated with the evaluation that caused us to 'complete' the cycle,
not the evaluatoon where we first discovered the cycle. As a result,
future reads from the evaluation cache would miss some incremental
compilation dependencies that would have otherwise been added if the
evaluation was *not* cached.

Under the right circumstances, this could lead to us trying to force
a query with a no-longer-existing `DefPathHash`, since we were missing
the (red) dependency edge that would have caused us to bail out before
attempting forcing.

This commit makes the provisional cache store the `DepNode` create
during the provisional evaluation. When we move an entry from the
provisional cache to the evaluation cache, we create a *new* `DepNode`
that has dependencies going to *both* of the evaluation `DepNodes` we
have available. This ensures that cached reads will incur all of
the necessary dependency edges.
This was referenced Jan 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88794 (Add a `try_clone()` function to `OwnedFd`.)
 - rust-lang#93064 (Properly track `DepNode`s in trait evaluation provisional cache)
 - rust-lang#93118 (Move param count error emission to end of `check_argument_types`)
 - rust-lang#93144 (Work around missing code coverage data causing llvm-cov failures)
 - rust-lang#93169 (Fix inconsistency of local blanket impls)
 - rust-lang#93175 (Implement stable overlap check considering negative traits)
 - rust-lang#93251 (rustdoc settings: use radio buttons for theme)
 - rust-lang#93269 (Use error-on-mismatch policy for PAuth module flags.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cf70411 into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
@pnkfelix
Copy link
Member

should this have had a perf run? (I'm trying to track down what caused some performance deltas in rollup PR #93288.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trait selection provisional cache incorrectly handles DepNodes, leading to ICE
6 participants