Don't store locals in generators that are immediately overwritten with the resume argument #69716
Conversation
This slims down the generator MIR considerably, which makes debugging easier
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @tmandry |
| // analyses. In particular the dataflow/liveness analyses don't know that `Yield` only | ||
| // stores to the `resume_arg` place upon resumption (and never reads from it). | ||
| if resume_arg.projection.is_empty() { | ||
| live_locals_here.remove(resume_arg.local); |
There was a problem hiding this comment.
Should this case on whether or not the local being written to by the resume argument implements Drop and has not been invalidated? That is, let mut x = foo.lock(); x = yield; shouldn't release the lock until after the yield has returned a value, so we need to continue to store space for x in the generator state, right?
There was a problem hiding this comment.
I believe this is all handled by MIR construction / drop elaboration already, at this point MIR-level assignments just overwrite the old value without running any drop glue. Specifically in your example, the resume argument would be assigned to a temporary first, which then overwrites x after the old value has been dropped.
There was a problem hiding this comment.
I'd rather not rely on more implementation details of MIR construction, if possible.
There was a problem hiding this comment.
It's not really an implementation detail – assignments (and terminators that write to a place) simply don't drop the old value
There was a problem hiding this comment.
Sorry, after re-reading I think @cramertj's case was different than the one I had in mind (the one I commented on below).
| live_locals_here.remove(self_arg()); | ||
|
|
||
| // If the resume arg ends up in a plain local, we ignore the local's liveness in the |
There was a problem hiding this comment.
This assumption doesn't hold in cases where the resume arg is Drop. If the generator is dropped before resuming, we will need to read its value in the destructor.
In the remaining cases, it would be correct to say that the local isn't live across the yield point.
In fact, the existing liveness analysis should work if it considers the yield to be a def of its resume arg. Yields with resume args are already treated as stores on the resume arg, and it does the right thing for projections. So at first glance, it seems like we shouldn't need this change at all.
The reason it doesn't work today probably has to do with the liveness algorithm and where it "places" the effects (i.e. it is probably acting like a write occurs before the yield instead of after it). We may need something analogous to the dataflow framework's call_return_effect here.
There was a problem hiding this comment.
Yeah, my first attempt at fixing this tried to do exactly that (including adding a resume_effect), but it ended up just reintroducing #69039, so I took this simpler approach instead. I don't think I currently understand all the dataflow analyses that feed into the generator transform well enough, but I can look into this again if you want.
There was a problem hiding this comment.
Heh, turns out I forgot to call a function. Pushed some commits that adjust the dataflow framework/analysis. (cc @ecstatic-morse for that).
There was a problem hiding this comment.
I wasn't imagining doing this in dataflow, but in the liveness analysis. The yield should be modeled as a def that occurs on resume.
(Though now that I think about it, we need it in dataflow too, to model the assignment from the yield.)
There was a problem hiding this comment.
If I'm not mistaken that should already happen due to the MutatingUse classification. The issue with that is that liveness isn't using the dataflow framework for some reason, so it has no way to model events that only happen along certain edges in the CFG.
I don't entirely understand why we're using what essentially boils down to two liveness analyses here though, it seems to me that MaybeRequiresStorage is strictly more precise than liveness since it uses the dataflow framework? The generator transform requires that both analyses say a local is live to store it in the state anyways.
There was a problem hiding this comment.
Just catching up now.
My understanding is that MaybeRequiresStorage could basically be (liveness | maybe_borrowed) & storage_live, where liveness is the canonical backwards dataflow analysis and storage_live uses StorageLive/StorageDead to determine whether borrows of certain locals have been invalidated. Is this correct?
The changes to the dataflow framework are good (I might rename some stuff in a cleanup PR, but that's just my OCD). However, I think it is worthwhile to try to express MaybeRequiresStorage as a combination of simple component analyses as described above. Whether this PR should be merged before that attempt is made is up to y'all. It depends on how urgent the memory savings from this PR are. Since this PR is already complete, and it seems like we may at some point want a "yield-converges" dataflow effect anyways, I would lean towards merging it.
One clarification, the pass in utils/liveness.rs is still a dataflow pass, just not a forwards one. I can extend the framework to also handle backwards dataflow, but since generator storage is the only place that analysis is used, you should feel free to make changes to it directly. It's not a major refactor.
There was a problem hiding this comment.
Ah okay, I didn't realize it's the only backwards analysis. This PR isn't exactly urgent, but it's somewhat of a soft blocker for #69033, which I'd love to have available in nightly as soon as that seems reasonable. Thanks!
There was a problem hiding this comment.
I don't think liveness analysis would use the dataflow framework actually. As I recall, it's a fundamentally different computation. We'd need to extend what's there to support the same kind of success effects that dataflow supports, though.
EDIT: (sorry, I left this tab open and didn't see the most recent comments)
There was a problem hiding this comment.
@ecstatic-morse it might be possible to express that, but there's some extra stuff that MaybeRequiresStorage does today. If a value is moved from and it hasn't been borrowed, then it doesn't require storage. I'm not sure where to stick that logic in a refactor (if it goes in maybe_borrowed but not liveness for example, we'll optimize fewer cases).
Also fwiw, I believe @eddyb's previous work on NRVO involved bidirectional dataflow, so it might be worth introducing at some point :)
There was a problem hiding this comment.
You can think of a move from a local as x = undef for the purposes of the liveness analysis, i.e. x is "kill"-ed (no longer live) when it is moved out of.
We now have a way to apply an effect only *after* a `yield` resumes, similar to calls (which can either return or unwind).
|
@bors r+ |
|
📌 Commit b26e27c has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
|
Seems too dangerous to go into one, so... @bors rollup=never |
|
@bors p=1 |
|
☀️ Test successful - checks-azure |
This fixes #69672 and makes #69033 pass the async fn size tests again (in other words, there will be no size regression of async fn if both this and #69033 land).
This is a small botch and I'd rather have a more precise analysis, but that seems much harder to pull off, so this special-cases(this now properly updates the dataflow framework to handle this case)Yieldterminators that store the resume argument into a simple local (ie. without any field projections) and explicitly marks that local as "not live" in the suspend point of that yield. We know that this local does not need to be stored in the generator for this suspend point because the next resume would immediately overwrite it with the passed-in resume argument anyways. The local might still end up in the state if it is used across another yield.