Multi-variant layouts for generators #59897
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
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
tmandry
force-pushed
the
variantful-generators
branch
from
a15b0f0
to
9432b3f
Compare
This comment has been minimized.
This comment has been minimized.
tmandry
force-pushed
the
variantful-generators
branch
from
9432b3f
to
5b07b40
Compare
This comment has been minimized.
This comment has been minimized.
tmandry
force-pushed
the
variantful-generators
branch
from
5b07b40
to
167164f
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 |
|
One funky aspect of this is that we declare locals in debuginfo multiple times, one for each yield point their storage lives across (which, for now, is all of them). This will make more sense when/if we add scopes to those locals that correspond with the yield points. For now, I don't see any downsides to this, except bigger debuginfo. |
tmandry
force-pushed
the
variantful-generators
branch
from
db8e691
to
955b245
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 |
tmandry
force-pushed
the
variantful-generators
branch
from
955b245
to
2cb4fad
Compare
tmandry
changed the title
tmandry
changed the title
tmandry
commented
Apr 19, 2019
|
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 |
tmandry
changed the title
|
Travis failure is for the wrong commit. Can someone re-run? cc @rust-lang/infra |
|
@tmandry You could re-run it by closing & reopening the PR, or pushing a new commit. But the failure in https://travis-ci.com/rust-lang/rust/jobs/194180525 does look legit and referring to the correct HEAD commit 2cb4fadfb54e8d9a3a724f58829842bcc68927a5 (merge commit d656918bd26aa255a6288931ed16d51589f20ca8). |
|
@kennytm Nevermind I'm dumb, I forgot about merge commits, sorry. Still having trouble reproducing this on linux, even on the exact merge commit that's failing in Travis... |
tmandry
force-pushed
the
variantful-generators
branch
from
2cb4fad
to
b054710
Compare
|
Rebased in hopes that this error magically goes away :) |
|
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 |
10 tasks
|
☀️ Test successful - checks-travis, status-appveyor |
This was referenced May 4, 2019
|
This broke the generator traversal logic in Miri at rust/src/librustc_mir/interpret/visitor.rs Lines 315 to 323 in e232636 Any advise for how to fix it? |
|
Looks like even though generators have variants, they still also contain "wrong" fields in their variants, meaning fields that are not actually going to be initialized when that variant is active. So generators still need a special exception. I think for now I'll just treat the entire generator as if it was a union. Are there any plans to make the layout information correct, in the sense that when a field is present in a variant according to the layout, then it will also always be initialized? |
No. Generators are basically a function frame, not an enum. Even for enums it is not true as they can be partially moved out of. A similar thing can happen with generators where a value in the generator is partially moved out of, even when the generator is not executing. There might be other edge cases due to generators being function frames. #60187 should make the layout closer to the actual values contained though. |
|
Moving out is not a problem -- that leaves the bits in the old place untouched, so they still satisfy the validity invariant. What is a problem is having a @eddyb However, this does somehow conflict with your proposal to make |
|
@RalfJung Yeah, #60187 will get us closer. As @eddyb pointed out, we still need to save liveness (not just storage-liveness) information in The short-term fix we discussed will be to wrap the types of every field with |
Merged
That would also work (and will also do the right thing for inhabitedness and layout computation). |
Centril
added a commit
to Centril/rust
that referenced
this pull request
May 8, 2019
…-obk Add test for rust-lang#59972 This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897. Closes rust-lang#59972. r? @eddyb
Centril
added a commit
to Centril/rust
that referenced
this pull request
May 8, 2019
…-obk Add test for rust-lang#59972 This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897. Closes rust-lang#59972. r? @eddyb
Centril
added a commit
to Centril/rust
that referenced
this pull request
May 8, 2019
…-obk Add test for rust-lang#59972 This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897. Closes rust-lang#59972. r? @eddyb
Centril
added a commit
to Centril/rust
that referenced
this pull request
May 8, 2019
…-obk Add test for rust-lang#59972 This PR adds a test for rust-lang#59972, which was fixed in rust-lang#59897. Closes rust-lang#59972. r? @eddyb
This was referenced May 10, 2019
bors
added a commit
that referenced
this pull request
Jun 12, 2019
Generator optimization: Overlap locals that never have storage live at the same time The specific goal of this optimization is to optimize async fns which use `await!`. Notably, `await!` has an enclosing scope around the futures it awaits ([definition](https://github.com/rust-lang/rust/blob/08bfe16129b0621bc90184f8704523d4929695ef/src/libstd/macros.rs#L365-L381)), which we rely on to implement the optimization. More generally, the optimization allows overlapping the storage of some locals which are never storage-live at the same time. **We care about storage-liveness when computing the layout, because knowing a field is `StorageDead` is the only way to prove it will not be accessed, either directly or through a reference.** To determine whether we can overlap two locals in the generator layout, we look at whether they might *both* be `StorageLive` at any point in the MIR. We use the `MaybeStorageLive` dataflow analysis for this. We iterate over every location in the MIR, and build a bitset for each local of the locals it might potentially conflict with. Next, we assign every saved local to one or more variants. The variants correspond to suspension points, and we include the set of locals live across a given suspension point in the variant. (Note that we use liveness instead of storage-liveness here; this ensures that the local has actually been initialized in each variant it has been included in. If the local is not live across a suspension point, then it doesn't need to be included in that variant.). It's important to note that the variants are a "view" into our layout. For the layout computation, we use a simplified approach. 1. Start with the set of locals assigned to only one variant. The rest are disqualified. 2. For each pair of locals which may conflict *and are not assigned to the same variant*, we pick one local to disqualify from overlapping. Disqualified locals go into a non-overlapping "prefix" at the beginning of our layout. This means they always have space reserved for them. All the locals that are allowed to overlap in each variant are then laid out after this prefix, in the "overlap zone". So, if A and B were disqualified, and X, Y, and Z were all eligible for overlap, our generator might look something like this: You can think of a generator as an enum, where some fields are shared between variants. e.g. ```rust enum Generator { Unresumed, Poisoned, Returned, Suspend0(A, B, X), Suspend1(B), Suspend2(A, Y, Z), } ``` where every mention of `A` and `B` refer to the same field, which does not move when changing variants. Note that `A` and `B` would automatically be sent to the prefix in this example. Assuming that `X` is never `StorageLive` at the same time as either `Y` or `Z`, it would be allowed to overlap with them. Note that if two locals (`Y` and `Z` in this case) are assigned to the same variant in our generator, their memory would never overlap in the layout. Thus they can both be eligible for the overlapping section, even if they are storage-live at the same time. --- Depends on: - [x] #59897 Multi-variant layouts for generators - [x] #60840 Preserve local scopes in generator MIR - [x] #61373 Emit StorageDead along unwind paths for generators Before merging: - [x] ~Wrap the types of all generator fields in `MaybeUninitialized` in layout::ty::field~ (opened #60889) - [x] Make PR description more complete (e.g. explain why storage liveness is important and why we have to check every location) - [x] Clean up TODO - [x] Fix the layout code to enforce that the same field never moves around in the generator - [x] Add tests for async/await - [x] ~Reduce # bits we store by half, since the conflict relation is symmetric~ (note: decided not to do this, for simplicity) - [x] Store liveness information for each yield point in our `GeneratorLayout`, that way we can emit more useful debuginfo AND tell miri which fields are definitely initialized for a given variant (see discussion at #59897 (comment))
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 12, 2019
…ddyb Generator optimization: Overlap locals that never have storage live at the same time The specific goal of this optimization is to optimize async fns which use `await!`. Notably, `await!` has an enclosing scope around the futures it awaits ([definition](https://github.com/rust-lang/rust/blob/08bfe16129b0621bc90184f8704523d4929695ef/src/libstd/macros.rs#L365-L381)), which we rely on to implement the optimization. More generally, the optimization allows overlapping the storage of some locals which are never storage-live at the same time. **We care about storage-liveness when computing the layout, because knowing a field is `StorageDead` is the only way to prove it will not be accessed, either directly or through a reference.** To determine whether we can overlap two locals in the generator layout, we look at whether they might *both* be `StorageLive` at any point in the MIR. We use the `MaybeStorageLive` dataflow analysis for this. We iterate over every location in the MIR, and build a bitset for each local of the locals it might potentially conflict with. Next, we assign every saved local to one or more variants. The variants correspond to suspension points, and we include the set of locals live across a given suspension point in the variant. (Note that we use liveness instead of storage-liveness here; this ensures that the local has actually been initialized in each variant it has been included in. If the local is not live across a suspension point, then it doesn't need to be included in that variant.). It's important to note that the variants are a "view" into our layout. For the layout computation, we use a simplified approach. 1. Start with the set of locals assigned to only one variant. The rest are disqualified. 2. For each pair of locals which may conflict *and are not assigned to the same variant*, we pick one local to disqualify from overlapping. Disqualified locals go into a non-overlapping "prefix" at the beginning of our layout. This means they always have space reserved for them. All the locals that are allowed to overlap in each variant are then laid out after this prefix, in the "overlap zone". So, if A and B were disqualified, and X, Y, and Z were all eligible for overlap, our generator might look something like this: You can think of a generator as an enum, where some fields are shared between variants. e.g. ```rust enum Generator { Unresumed, Poisoned, Returned, Suspend0(A, B, X), Suspend1(B), Suspend2(A, Y, Z), } ``` where every mention of `A` and `B` refer to the same field, which does not move when changing variants. Note that `A` and `B` would automatically be sent to the prefix in this example. Assuming that `X` is never `StorageLive` at the same time as either `Y` or `Z`, it would be allowed to overlap with them. Note that if two locals (`Y` and `Z` in this case) are assigned to the same variant in our generator, their memory would never overlap in the layout. Thus they can both be eligible for the overlapping section, even if they are storage-live at the same time. --- Depends on: - [x] rust-lang#59897 Multi-variant layouts for generators - [x] rust-lang#60840 Preserve local scopes in generator MIR - [x] rust-lang#61373 Emit StorageDead along unwind paths for generators Before merging: - [x] ~Wrap the types of all generator fields in `MaybeUninitialized` in layout::ty::field~ (opened rust-lang#60889) - [x] Make PR description more complete (e.g. explain why storage liveness is important and why we have to check every location) - [x] Clean up TODO - [x] Fix the layout code to enforce that the same field never moves around in the generator - [x] Add tests for async/await - [x] ~Reduce # bits we store by half, since the conflict relation is symmetric~ (note: decided not to do this, for simplicity) - [x] Store liveness information for each yield point in our `GeneratorLayout`, that way we can emit more useful debuginfo AND tell miri which fields are definitely initialized for a given variant (see discussion at rust-lang#59897 (comment))
Merged
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 14, 2019
Issue rust-lang#60709 test Adds a test for rust-lang#60709, which was fixed as part of rust-lang#59897. r? @cramertj
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 29, 2019
Issue rust-lang#60709 test Adds a test for rust-lang#60709, which was fixed as part of rust-lang#59897. r? @cramertj
This was referenced Jul 10, 2019
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 allows generators to overlap fields using variants, but doesn't do any such overlapping yet. It creates one variant for every state of the generator (unresumed, returned, panicked, plus one for every yield), and puts every stored local in each of the yield-point variants.
Required for optimizing generator layouts (#52924).
There was quite a lot of refactoring needed for this change. I've done my best in later commits to eliminate assumptions in the code that only certain kinds of types are multi-variant, and to centralize knowledge of the inner mechanics of generators in as few places as possible.
This change also emits debuginfo about the fields contained in each variant, as well as preserving debuginfo about stored locals while running in the generator.
Also, fixes #59972.
Future work: