Projection cache and better warnings for #32330 #33816
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bluss
added
the
relnotes
|
I've got (what I believe to be) an improved approach that seems to perform better. Basically not caching the obligations. But I'm not done making measurements yet. |
nikomatsakis
force-pushed
the
projection-cache-2
branch
from
295dc3f to
c2d6d38
Compare
|
Performance results with latest fix from the test case in issue #31849: |
|
Ah, I forgot to mention, these are the results from a crater run where the lints were set to Deny: |
|
|
||
| // Check that you are allowed to implement using elision but write | ||
| // trait without elision (a bug in this cropped up during | ||
| // bootstrapping, so this is a regression test). |
There was a problem hiding this comment.
How did normalization get involved here?
There was a problem hiding this comment.
@arielb1 I think it had to do w/ earlier revisions of the code that classifies regions as early-vs-late-bound.
|
☔ The latest upstream changes (presumably #33602) made this pull request unmergeable. Please resolve the merge conflicts. |
Currently, we consider region subtyping a failure if a skolemized lifetime is relatable to any other lifetime in any way at all. But a more precise formulation is to say that a skolemized lifetime: - must not have any *incoming* edges in the region graph - only has *outgoing* edges to nodes that are `'static` To enforce the latter requirement, we add edges from `'static -> 'x` for each lifetime '`x' reachable from a skolemized region. We now have to add a new `pop_skolemized` routine to do cleanup. Whereas before if there were *any* edges relating to a skolemized region, we would return `Err` and hence rollback the transaction, we now tolerate some edges and return `Ok`. Therefore, the `pop_skolemized` routine runs and cleans up those edges.
This indicates whether this `BoundRegion` will change from late to early bound when issue 32330 is fixed. It also indicates the function on which the lifetime is declared.
When we do a "HR subtype" check, we replace all late-bound regions (LBR)
in the subtype with fresh variables, and skolemize the late-bound
regions in the supertype. If those skolemized regions from the supertype
wind up being super-regions (directly or indirectly) of either
- another skolemized region; or,
- some region that pre-exists the HR subtype check
- e.g., a region variable that is not one of those created
to represent bound regions in the subtype
then the subtype check fails.
What will change when we fix rust-lang#32330 is that some of the LBR in the
subtype may become early-bound. In that case, they would no longer be in
the "permitted set" of variables that can be related to a skolemized
type.
So the foundation for this warning is to collect variables that we found
to be related to a skolemized type. For each of them, we have a
`BoundRegion` which carries a `Issue32330` flag. We check whether any of
those flags indicate that this variable was created from a lifetime
that will change from late- to early-bound. If so, we issue a warning
indicating that the results of compilation may change.
This is imperfect, since there are other kinds of code that will not
compile once rust-lang#32330 is fixed. However, it fixes the errors observed in
practice on crater runs.
A lot of the refactors, however, seem helpful, so leave those in, particularly since we may want to make this change in the future.
We used to make region->region edges part of the verify set, but this change stores them like other edges, as a full-fledged constraint. Besides making the code somewhat cleaner, this allows them to be more easily dropped as part of `pop_skolemized`. This change also refactors the code a bit to remove some intermediate data structures that are no longer particular useful (e.g., VarValue).
we used to have two separate routines, one in tyencode/tydecode, and one in encode/decode.
Currently, when projecting out of a higher-ranked where-clause, we instantiate all higher-ranked regions with lifetime variables. This is unnecessary since the language rules ought to guarantee (modulo rust-lang#32330) that each of those higher-ranked regions is equated with some regions from the input types. This routine figures out what those regions are and just uses them. Also, since rust-lang#32330 is not fully fixed, it detects when we may have unconstrained variables and indicates that in its return value.
also, consolidate the return type into from a tuple into a struct `Progress`
the vtable.nested obligations were being dropped on the floor.
in some cases we give more specific messages or fewer duplicates, now that we have cache and make fewer region variables
This test was abusing rust-lang#32330; cleanup the code some.
nikomatsakis
force-pushed
the
projection-cache-2
branch
from
64ac2f2 to
480d18c
Compare
|
Rebased. |
|
@bors r+ |
|
📌 Commit 480d18c has been approved by |
bors
added a commit
that referenced
this pull request
Jun 4, 2016
Projection cache and better warnings for #32330 This PR does three things: - it lays the groundwork for the more precise subtyping rules discussed in #32330, but does not enable them; - it issues warnings when the result of a leak-check or subtyping check relies on a late-bound region which will late become early-bound when #32330 is fixed; - it introduces a cache for projection in the inference context. I'm not 100% happy with the approach taken by the cache here, but it seems like a step in the right direction. It results in big wins on some test cases, but not as big as previous versions -- I think because it is caching the `Vec<Obligation>` (whereas before I just returned the normalized type with an empty vector). However, that change was needed to fix an ICE in @alexcrichton's future-rs module (I haven't fully tracked the cause of that ICE yet). Also, because trans/the collector use a fresh inference context for every call to `fulfill_obligation`, they don't profit nearly as much from this cache as they ought to. Still, here are the results from the future-rs `retry.rs`: ``` 06:26 <nmatsakis> time: 6.246; rss: 44MB item-bodies checking 06:26 <nmatsakis> time: 54.783; rss: 63MB translation item collection 06:26 <nmatsakis> time: 140.086; rss: 86MB translation 06:26 <nmatsakis> time: 0.361; rss: 46MB item-bodies checking 06:26 <nmatsakis> time: 5.299; rss: 63MB translation item collection 06:26 <nmatsakis> time: 12.140; rss: 86MB translation ``` ~~Another example is the example from #31849. For that, I get 34s to run item-bodies without any cache. The version of the cache included here takes 2s to run item-bodies type-checking. An alternative version which doesn't track nested obligations takes 0.2s, but that version ICEs on @alexcrichton's future-rs (and may well be incorrect, I've not fully convinced myself of that). So, a definite win, but I think there's definitely room for further progress.~~ Pushed a modified version which improves performance of the case from #31849: ``` lunch-box. time rustc --stage0 ~/tmp/issue-31849.rs -Z no-trans real 0m33.539s user 0m32.932s sys 0m0.570s lunch-box. time rustc --stage2 ~/tmp/issue-31849.rs -Z no-trans real 0m0.195s user 0m0.154s sys 0m0.042s ``` Some sort of cache is also needed for unblocking further work on lazy normalization, since that will lean even more heavily on the cache, and will also require cycle detection. r? @arielb1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does three things:
I'm not 100% happy with the approach taken by the cache here, but it seems like a step in the right direction. It results in big wins on some test cases, but not as big as previous versions -- I think because it is caching the
Vec<Obligation>(whereas before I just returned the normalized type with an empty vector). However, that change was needed to fix an ICE in @alexcrichton's future-rs module (I haven't fully tracked the cause of that ICE yet). Also, because trans/the collector use a fresh inference context for every call tofulfill_obligation, they don't profit nearly as much from this cache as they ought to.Still, here are the results from the future-rs
retry.rs:Another example is the example from #31849. For that, I get 34s to run item-bodies without any cache. The version of the cache included here takes 2s to run item-bodies type-checking. An alternative version which doesn't track nested obligations takes 0.2s, but that version ICEs on @alexcrichton's future-rs (and may well be incorrect, I've not fully convinced myself of that). So, a definite win, but I think there's definitely room for further progress.Pushed a modified version which improves performance of the case from #31849:
Some sort of cache is also needed for unblocking further work on lazy normalization, since that will lean even more heavily on the cache, and will also require cycle detection.
r? @arielb1