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

Avoid GenFuture shim when compiling async constructs #104321

Merged
merged 1 commit into from
Nov 24, 2022

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Nov 12, 2022

Previously, async constructs would be lowered to "normal" generators, with an additional from_generator / GenFuture shim in between to convert from Generator to Future.

The compiler will now special-case these generators internally so that async constructs will directly implement Future without the need to go through the from_generator / GenFuture shim.

The primary motivation for this change was hiding this implementation detail in stack traces and debuginfo, but it can in theory also help the optimizer as there is less abstractions to see through.


Given this demo code:

pub async fn a(arg: u32) -> Backtrace {
    let bt = b().await;
    let _arg = arg;
    bt
}

pub async fn b() -> Backtrace {
    Backtrace::force_capture()
}

I would get the following with the latest stable compiler (on Windows):

   4: async_codegen::b::async_fn$0
             at .\src\lib.rs:10
   5: core::future::from_generator::impl$1::poll<enum2$<async_codegen::b::async_fn_env$0> >
             at /rustc/897e37553bba8b42751c67658967889d11ecd120\library\core\src\future\mod.rs:91
   6: async_codegen::a::async_fn$0
             at .\src\lib.rs:4
   7: core::future::from_generator::impl$1::poll<enum2$<async_codegen::a::async_fn_env$0> >
             at /rustc/897e37553bba8b42751c67658967889d11ecd120\library\core\src\future\mod.rs:91

whereas now I get a much cleaner stack trace:

   3: async_codegen::b::async_fn$0
             at .\src\lib.rs:10
   4: async_codegen::a::async_fn$0
             at .\src\lib.rs:4

@rustbot
Copy link
Collaborator

rustbot commented Nov 12, 2022

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 12, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Swatinem
Copy link
Contributor Author

@cjgillot thanks for the review.

I implemented the feedback, defining separate structures / functions for the future case in trait selection.

I also fixed a couple of tests and blessed some test assertions that are clear improvements:

  • better spans for async blocks, (now covers the whole async {} instead of just the {} part)
  • tests that assumed from_generator wrapping (including moves)
  • removed a couple of error[E0391]: cycle detected when computing type of … errors, though I’m not entirely confident about that one.

Maybe I missed a couple of things though.


The main regression that now remains is, and that you pointed out on zulip:

error[E0700]: hidden type for `impl Future<Output = _>` captures lifetime that does not appear in bounds

This may be related to another change in diagnostics:

-	   = note: required because it captures the following types: `ResumeTy`, `(NotSend,)`, `impl Future<Output = ()>`, `()`
+	   = note: required because it captures the following types: `&mut Context<'_>`, `(NotSend,)`, `impl Future<Output = ()>`, `()`

As this now avoids the whole ResumeTy in favor of using the Context directly, that might be a reason for both these changes.

ResumeTy as it exists today in the shim I believe has the purpose to hide all these lifetimes behind some raw pointers and unsafe code to then conjure up the lifetimes again out of thin air in get_context:

pub unsafe fn get_context<'a, 'b>(cx: ResumeTy) -> &'a mut Context<'b> {

As I was asking on zulip: Do we need this at all, or can we just avoid checking the resume type at all? I’m also wondering why the generator is "capturing" that type at all? It shouldn’t be?


As a separate question: Should I remove the whole from_generator glue code from the std library right away, or rather wait one cycle in case some external code might rely on that directly?

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 14, 2022

r? @cjgillot

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot assigned cjgillot and unassigned oli-obk Nov 14, 2022
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 14, 2022
@bors
Copy link
Contributor

bors commented Nov 14, 2022

⌛ Trying commit 69930d3 with merge eaf9cebf4a95bc065d5aee8056b24fc910ac20e0...

@bors
Copy link
Contributor

bors commented Nov 14, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2022
@rust-log-analyzer

This comment has been minimized.

@Swatinem
Copy link
Contributor Author

I switched back to using ResumeTy and get_context which solves most of the new E0700 issues.

I’m down to 12 failures:

  • 6 are adding a new note that I believe I can figure out how to get rid of
  • 2 now falsely assume async blocks are const. well as they are "just data", they are, but there is still that feature flag for it I should respect ;-)
  • src/test\ui\generic-associated-types\bugs\issue-100013.rs suggests adding a async move, though not sure where that is coming from.

That leaves:

  • src/test\ui\regions\issue-72051-member-region-hang.rs and src/test\ui\self\self_lifetime-async.rs are complaining about E0700 captured lifetimes.
  • src/test\ui\impl-trait\in-trait\default-body-with-rpit.rs now has a E0720 cannot resolve opaque type.

I believe for these I might need to hook up the return type in a different way somehow, not yet sure how to do that.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 17, 2022

☔ The latest upstream changes (presumably #104219) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 18, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 1, 2022
Avoid `GenFuture` shim when compiling async constructs

Previously, async constructs would be lowered to "normal" generators, with an additional `from_generator` / `GenFuture` shim in between to convert from `Generator` to `Future`.

The compiler will now special-case these generators internally so that async constructs will *directly* implement `Future` without the need to go through the `from_generator` / `GenFuture` shim.

The primary motivation for this change was hiding this implementation detail in stack traces and debuginfo, but it can in theory also help the optimizer as there is less abstractions to see through.

---

Given this demo code:

```rust
pub async fn a(arg: u32) -> Backtrace {
    let bt = b().await;
    let _arg = arg;
    bt
}

pub async fn b() -> Backtrace {
    Backtrace::force_capture()
}
```

I would get the following with the latest stable compiler (on Windows):

```
   4: async_codegen::b::async_fn$0
             at .\src\lib.rs:10
   5: core::future::from_generator::impl$1::poll<enum2$<async_codegen::b::async_fn_env$0> >
             at /rustc/897e37553bba8b42751c67658967889d11ecd120\library\core\src\future\mod.rs:91
   6: async_codegen::a::async_fn$0
             at .\src\lib.rs:4
   7: core::future::from_generator::impl$1::poll<enum2$<async_codegen::a::async_fn_env$0> >
             at /rustc/897e37553bba8b42751c67658967889d11ecd120\library\core\src\future\mod.rs:91
```

whereas now I get a much cleaner stack trace:

```
   3: async_codegen::b::async_fn$0
             at .\src\lib.rs:10
   4: async_codegen::a::async_fn$0
             at .\src\lib.rs:4
```
@bjorn3
Copy link
Member

bjorn3 commented Dec 4, 2022

As of this PR cg_clif's CI is failing with

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `&mut std::task::Context<'_>`,
 right: `std::future::ResumeTy`: Can't write value with incompatible type Ref(ReErased, std::task::Context<'_>, Mut) to place with type Adt(std::future::ResumeTy, [])

It seems that an &mut Context<'_> is passed to poll function expecting ResumeTy. Both types are currently abi compatible by accident, but -Zrandomize-layout will break this in the future once it starts randomly adding padding to types.

@bjorn3
Copy link
Member

bjorn3 commented Dec 4, 2022

And after a patch to make it ignore this particular mismatch I get:

 thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `std::ops::GeneratorState<(), ()>`,
 right: `std::task::Poll<()>`: Can't write value with incompatible type Adt(std::ops::GeneratorState, [(), ()]) to place with type Adt(std::task::Poll, [()])

Poking a bit around confirms that <[async fn body@example/std_example.rs:20:19: 20:21] as std::future::Future>::poll does indeed accept ResumeTy and return std::ops::GeneratorState<(), ()> in violation of the Future::poll signature.

[src/abi/mod.rs:402] &instance = Some(Instance {
    def: Item(WithOptConstParam { did: DefId(0:20 ~ std_example[7fb1]::future::{closure#0}), const_param_did: None }),
    substs: [std::future::ResumeTy, (), (), {}, ()],
})
[src/abi/mod.rs:402] &fn_abi = FnAbi {
    args: [
        ArgAbi {
            layout: TyAndLayout {
                ty: std::pin::Pin<&mut [async fn body@example/std_example.rs:20:19: 20:21]>,
                layout: ...,
            },
            mode: Direct(...),
        },
        ArgAbi {
            layout: TyAndLayout {
                ty: std::future::ResumeTy,
                layout: ...,
            },
            mode: Direct(...),
        },
    ],
    ret: ArgAbi {
        layout: TyAndLayout {
            ty: std::ops::GeneratorState<(), ()>,
            layout: ...,
        },
        mode: Direct(...),
    },
    c_variadic: false,
    fixed_count: 2,
    conv: Rust,
    can_unwind: false,
}

A minimal reproducing example is:

#![feature(pin_macro)]

use std::pin::{pin, Pin};
use std::future::Future;
use std::ptr::null;
use std::task::{Context, Waker, RawWaker, RawWakerVTable};

fn main() {
    let waker = unsafe {
        Waker::from_raw(RawWaker::new(null(), &RawWakerVTable::new(|_| todo!(), |_| todo!(), |_| todo!(), |_| todo!())))
    };
    let mut cx = Context::from_waker(&waker);
    call_future(pin!(future()), &mut cx);
}

fn call_future(fut: Pin<&mut impl Future<Output = ()>>, cx: &mut Context) {
    fut.poll(cx);
}

async fn future() {}

@oli-obk
Copy link
Contributor

oli-obk commented Dec 4, 2022

This is being fixed in #105082 (comment)

@bjorn3
Copy link
Member

bjorn3 commented Dec 4, 2022

Thanks for the pointer!

@Swatinem
Copy link
Contributor Author

Swatinem commented Dec 4, 2022

#105082 will indeed fix the mismatch of GeneratorState return type.

I will have a fix for #104828 up shortly that will tackle the ResumeTy mismatch.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2022
…piler-errors

Make sure async constructs do not `impl Generator`

Async lowering turns async functions and blocks into generators internally.
Though these special kinds of generators should not `impl Generator` themselves.
The other way around, normal generators should not `impl Future`.

This was discovered in rust-lang#105082 (comment) and is a regression from rust-lang#104321.

r? `@compiler-errors`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 5, 2022
Make sure async constructs do not `impl Generator`

Async lowering turns async functions and blocks into generators internally.
Though these special kinds of generators should not `impl Generator` themselves.
The other way around, normal generators should not `impl Future`.

This was discovered in rust-lang/rust#105082 (comment) and is a regression from rust-lang/rust#104321.

r? `@compiler-errors`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? ``@oli-obk``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? ```@oli-obk```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? `@oli-obk`
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang/rust#104828 and rust-lang/rust#104321 (comment)

r? `@oli-obk`
Swatinem added a commit to Swatinem/rust that referenced this pull request May 7, 2023
This function/lang_item was introduced in rust-lang#104321 as a temporary workaround of future lowering.
The usage and need for it went away in rust-lang#104833.
After a bootstrap update, the function itself can be removed from `std`.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 8, 2023
…rk-Simulacrum

Remove `identity_future` from stdlib

This function/lang_item was introduced in rust-lang#104321 as a temporary workaround of future lowering. The usage and need for it went away in rust-lang#104833.
After a bootstrap update, the function itself can be removed from `std`.
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
This function/lang_item was introduced in rust-lang#104321 as a temporary workaround of future lowering.
The usage and need for it went away in rust-lang#104833.
After a bootstrap update, the function itself can be removed from `std`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 29, 2023
…th-late-bound-vars-again, r=jackh726

Normalize opaques with late-bound vars again

We have a hack in the compiler where if an opaque has escaping late-bound vars, we skip revealing it even though we *could* reveal it from a technical perspective. First of all, this is weird, since we really should be revealing all opaques in `Reveal::All` mode. Second of all, it causes subtle bugs (linked below).

I attempted to fix this in rust-lang#100980, which was unfortunately reverted due to perf regressions on codebases that used really deeply nested futures in some interesting ways. The worst of which was rust-lang#103423, which caused the project to hang on build. Another one was rust-lang#104842, which was just a slow-down, but not a hang. I took some time afterwards to investigate how to rework `normalize_erasing_regions` to take advantage of better caching, but that effort kinda fizzled out (rust-lang#104133).

However, recently, I was made aware of more bugs whose root cause is not revealing opaques during codegen. That made me want to fix this again -- in the process, interestingly, I took the the minimized example from rust-lang#103423 (comment), and it doesn't seem to hang any more...

Thinking about this harder, there have been some changes to the way we lower and typecheck async futures that may have reduced the pathologically large number of outlives obligations (see description of rust-lang#103423) that we were encountering when normalizing opaques with bound vars the last time around:
* rust-lang#104321 (lower `async { .. }` directly as a generator that implements `Future`, removing the `from_generator` shim)
* rust-lang#104833 (removing an `identity_future` fn that was wrapping desugared future generators)

... so given that I can see:
* No significant regression on rust perf bot (rust-lang#107620 (comment))
* No timeouts in crater run I did (rust-lang#107620 (comment), rechecked failing crates in rust-lang#107620 (comment))

... and given that this PR:
* Fixes rust-lang#104601
* Fixes rust-lang#107557
* Fixes rust-lang#109464
* Allows us to remove a `DefiningAnchor::Bubble` from codegen (75a8f68)

I'm inclined to give this another shot at landing this. Best case, it just works -- worst case, we get more examples to study how we need to improve the compiler to make this work.

r? types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants