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

integrate anon dep nodes into trait selection #43184

Merged

Conversation

nikomatsakis
Copy link
Contributor

Use anonymous nodes for trait selection. In all cases, we use the same basic "memoization" strategy:

  • Store the DepNodeIndex in the slot along with value.
  • If value is present, return it, and add a read of the dep-node-index.
  • Else, start an anonymous task, and store resulting node.

We apply this strategy to a number of caches in trait selection:

  • The "trans" caches of selection and projection
  • The "evaluation" cache
  • The "candidate selection" cache

In general, for our cache strategy to be "dep-correct", the computation of the value is permitted to rely on the value in the key but nothing else. The basic argument is this: in order to look something up, you have to produce the key, and to do that you must have whatever reads were needed to create the key. Then, you get whatever reads were further needed to produce the value. But if the "closure" that produced the value made use of other environmental data, not derivable from the key, that would be bad -- but that would also suggest that the cache is messed up (though it's not proof).

The structure of these caches do not obviously prove that the correctness criteria are met, and I aim to address that in further refactorings. But I believe it to be the case that, if we assume that the existing caches are correct, there are also no dependency failures (in other words, if there's a bug, it's a pre-existing one). Specifically:

  • The trans caches: these take as input just a tcx, which is "by definition" not leaky, the trait-ref etc, which is part of the key, and sometimes a span (doesn't influence the result). So they seem fine.
  • The evaluation cache:
    • This computation takes as input the "stack" and has access to the infcx.
    • The infcx is a problem -- would be better to take the tcx -- and this is exactly one of the things I plan to improve in later PRs. Let's ignore it for now. =)
    • The stack itself is also not great, in that the key only consists of the top-entry in the stack.
    • However, the stack can cause a problem in two ways:
      • overflow (we panic)
      • cycle check fails (we do not update the cache, I believe)
  • The candidate selection cache:
    • as before, takes the "stack" and has access to the infcx.
    • here it is not as obvious that we avoid caching stack-dependent computations. However, to the extent that we do, this is a pre-existing bug, in that we are making cache entries we shouldn't.
    • I aim to resolve this by -- following the chalk-style of evaluation -- merging candidate selection and evaluation.
    • The infcx is a problem -- would be better to take the tcx -- and this is exactly one of the things I plan to improve in later PRs. Let's ignore it for now. =)
    • The stack itself is also not great, in that the key only consists of the top-entry in the stack.
    • Moreover, the stack would generally just introduce ambiguities and errors anyhow, so that lessens the risk.

Anyway, the existing approach to handle dependencies in the trait code carries the same risks or worse, so this seems like a strict improvement!

r? @michaelwoerister

cc @arielb1

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2017
@michaelwoerister
Copy link
Member

@bors r+

Excellent! This should bring us a lot closer to red-green evaluation.

@bors
Copy link
Contributor

bors commented Jul 13, 2017

📌 Commit 4f030d0 has been approved by michaelwoerister

@bors
Copy link
Contributor

bors commented Jul 15, 2017

⌛ Testing commit 4f030d0 with merge b4502f7...

bors added a commit that referenced this pull request Jul 15, 2017
…on, r=michaelwoerister

integrate anon dep nodes into trait selection

Use anonymous nodes for trait selection. In all cases, we use the same basic "memoization" strategy:

- Store the `DepNodeIndex` in the slot along with value.
- If value is present, return it, and add a read of the dep-node-index.
- Else, start an anonymous task, and store resulting node.

We apply this strategy to a number of caches in trait selection:

- The "trans" caches of selection and projection
- The "evaluation" cache
- The "candidate selection" cache

In general, for our cache strategy to be "dep-correct", the computation of the value is permitted to rely on the *value in the key* but nothing else. The basic argument is this: in order to look something up, you have to produce the key, and to do that you must have whatever reads were needed to create the key. Then, you get whatever reads were further needed to produce the value. But if the "closure" that produced the value made use of *other* environmental data, not derivable from the key, that would be bad -- but that would **also** suggest that the cache is messed up (though it's not proof).

The structure of these caches do not obviously prove that the correctness criteria are met, and I aim to address that in further refactorings. But I *believe* it to be the case that, if we assume that the existing caches are correct, there are also no dependency failures (in other words, if there's a bug, it's a pre-existing one). Specifically:

- The trans caches: these take as input just a `tcx`, which is "by definition" not leaky, the `trait-ref` etc, which is part of the key, and sometimes a span (doesn't influence the result). So they seem fine.
- The evaluation cache:
    - This computation takes as input the "stack" and has access to the infcx.
    - The infcx is a problem -- would be better to take the tcx -- and this is exactly one of the things I plan to improve in later PRs. Let's ignore it for now. =)
    - The stack itself is also not great, in that the *key* only consists of the top-entry in the stack.
    - However, the stack can cause a problem in two ways:
        - overflow (we panic)
        - cycle check fails (we do not update the cache, I believe)
- The candidate selection cache:
    - as before, takes the "stack" and has access to the infcx.
    - here it is not as obvious that we avoid caching stack-dependent computations. However, to the extent that we do, this is a pre-existing bug, in that we are making cache entries we shouldn't.
    - I aim to resolve this by -- following the chalk-style of evaluation -- merging candidate selection and evaluation.
    - The infcx is a problem -- would be better to take the tcx -- and this is exactly one of the things I plan to improve in later PRs. Let's ignore it for now. =)
    - The stack itself is also not great, in that the *key* only consists of the top-entry in the stack.
    - Moreover, the stack would generally just introduce ambiguities and errors anyhow, so that lessens the risk.

Anyway, the existing approach to handle dependencies in the trait code carries the same risks or worse, so this seems like a strict improvement!

r? @michaelwoerister

cc @arielb1
@bors
Copy link
Contributor

bors commented Jul 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing b4502f7 to master...

@bors bors merged commit 4f030d0 into rust-lang:master Jul 15, 2017
@alexcrichton
Copy link
Member

This decreased the memory usage of two benchmarks by 8% and improved compile time of one benchmark by up to 40%.

Nice! 🎉

arielb1 added a commit to arielb1/rust that referenced this pull request Aug 17, 2017
…ize-trait-selection, r=michaelwoerister"

This reverts commit b4502f7, reversing
changes made to 23ecebd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants