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

x = yield; makes generators larger than they need to be #69672

Closed
jonas-schievink opened this issue Mar 3, 2020 · 6 comments · Fixed by #69716
Closed

x = yield; makes generators larger than they need to be #69672

jonas-schievink opened this issue Mar 3, 2020 · 6 comments · Fixed by #69716
Assignees
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. F-coroutines `#![feature(coroutines)]` I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Mar 3, 2020

This generator has a size of 4 Bytes:

    let mut gen = |mut x| {
        loop {
            drop(x);
            yield;
            x = makeit();
            useit(&x);
        }
    };

This one is 16 Bytes in size, even though it also does not need to keep x alive across the yield:

    let mut gen = |mut x| {
        loop {
            drop(x);
            x = yield;
            useit(&x);
        }
    };

(where makeit is a fn returning a usize, and useit is a fn taking &usize)

This seems to be fallout from #69302. Either the layout calculation soundness fix exposed it, or the visitor changes in there caused it.

This means that #69033 will increase the size of futures created from async fns, unless this bug is fixed.

@jonas-schievink jonas-schievink added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-coroutines Area: Coroutines I-heavy Issue: Problems and improvements with respect to binary size of generated code. A-async-await Area: Async & Await F-coroutines `#![feature(coroutines)]` labels Mar 3, 2020
@hanna-kruppe
Copy link
Contributor

FWIW the evaluation order of <place> = <expr> (at MIR level) is currently: first evaluate <expr> (into a temporary), then drop <place>. For types without drop glue (like usize) that shouldn't matter, and even for types with drop glue there's reasons to change that if we can (to be able to avoid temporaries). But still, the evaluation order being this way currently means x would, in fact, have to be kept alive across the yield if it had drop glue or might have drop glue (since MIR is polymorphic, not monomorphized, and the generator->state machine transform happens on MIR).

@jonas-schievink
Copy link
Contributor Author

True, but shouldn't the unconditional drop(x); before the yield prevent that? The value wouldn't be live anymore when the yield happens.

(there's also a separate bug where a type being Copy can actually make a generator larger, but that also shouldn't happen here)

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Mar 3, 2020

Oh, I missed that somehow. That liveness is not taken into account for generator captures is a problem independent of resume arguments, see #69663. Could async fns after #69033 have such an explicit drop too or would they still be affected if #69663 was fixed?

@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Mar 3, 2020
@jonas-schievink
Copy link
Contributor Author

#69663 is about typeck's view of captures from what I can tell. The transform itself (which determines the memory layout) runs on optimized MIR and should already be able to tell whether variables are really live across yield points.

@tmandry
Copy link
Member

tmandry commented Mar 3, 2020

@rustbot claim

Assigning myself to think about this issue and leave notes on the underlying issues involved.

@jonas-schievink
Copy link
Contributor Author

I've got a fix up in #69716 that should cover most cases here.

@bors bors closed this as completed in 5ed3453 Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. F-coroutines `#![feature(coroutines)]` I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants