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

Remove some stack frames from .async calls #66398

Merged
merged 1 commit into from
Nov 15, 2019

Conversation

sfackler
Copy link
Member

The Context argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

  1. get_task_context has been inlined into poll_with_tls_context,
    removing 2 frames (16 and 17 above).
  2. set_task_context now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing poll_with_tls_context in favor
of a get_task_context function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.

The `Context` argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

```
  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6
```

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

1. `get_task_context` has been inlined into `poll_with_tls_context`,
    removing 2 frames (16 and 17 above).
2. `set_task_context` now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing `poll_with_tls_context` in favor
of a `get_task_context` function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.
@rust-highfive
Copy link
Collaborator

r? @rkruppe

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2019
@Centril
Copy link
Contributor

Centril commented Nov 14, 2019

Thanks! r? @Centril @bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2019

📌 Commit 3fe7cfc has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 14, 2019
Remove some stack frames from `.async` calls

The `Context` argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

```
  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6
```

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

1. `get_task_context` has been inlined into `poll_with_tls_context`,
    removing 2 frames (16 and 17 above).
2. `set_task_context` now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing `poll_with_tls_context` in favor
of a `get_task_context` function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.
tmandry added a commit to tmandry/rust that referenced this pull request Nov 14, 2019
Remove some stack frames from `.async` calls

The `Context` argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

```
  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6
```

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

1. `get_task_context` has been inlined into `poll_with_tls_context`,
    removing 2 frames (16 and 17 above).
2. `set_task_context` now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing `poll_with_tls_context` in favor
of a `get_task_context` function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 15, 2019
Remove some stack frames from `.async` calls

The `Context` argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

```
  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6
```

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

1. `get_task_context` has been inlined into `poll_with_tls_context`,
    removing 2 frames (16 and 17 above).
2. `set_task_context` now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing `poll_with_tls_context` in favor
of a `get_task_context` function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 15, 2019
Remove some stack frames from `.async` calls

The `Context` argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

```
  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6
```

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

1. `get_task_context` has been inlined into `poll_with_tls_context`,
    removing 2 frames (16 and 17 above).
2. `set_task_context` now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing `poll_with_tls_context` in favor
of a `get_task_context` function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.
bors added a commit that referenced this pull request Nov 15, 2019
Rollup of 12 pull requests

Successful merges:

 - #65557 (rename Error::iter_chain() and remove Error::iter_sources())
 - #66013 (Avoid hashing the key twice in `get_query()`.)
 - #66306 (Remove cannot mutate statics in initializer of another static error)
 - #66338 (Update mdbook.)
 - #66388 (Do not ICE on recovery from unmet associated type bound obligation)
 - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`)
 - #66391 (Do not ICE in `if` without `else` in `async fn`)
 - #66394 (Fix two OOM issues related to `ConstProp`)
 - #66398 (Remove some stack frames from `.async` calls)
 - #66410 (miri: helper methods for max values of machine's usize/isize)
 - #66418 (Link to tracking issue in HIR const-check error code description)
 - #66419 (Don't warn labels beginning with `_` on unused_labels lint)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 15, 2019
Remove some stack frames from `.async` calls

The `Context` argument is currently smuggled through TLS for
async-generated futures. The current infrastructure is closure-based,
and results in an extra 6 stack frames when .awaiting an async-generated
future!

```
  12: foo::async_b::{{closure}}
             at src/main.rs:10
  13: <std::future::GenFuture<T> as core::future::future::Future>::poll::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  14: std::future::set_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:79
  15: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:43
  16: std::future::poll_with_tls_context::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  17: std::future::get_task_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:111
  18: std::future::poll_with_tls_context
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/future.rs:121
  19: foo::async_a::{{closure}}
             at src/main.rs:6
```

While the long (medium?) term solution is to remove the use of TLS
entirely, we can improve things a bit in the meantime. In particular,
this commit does 2 things:

1. `get_task_context` has been inlined into `poll_with_tls_context`,
    removing 2 frames (16 and 17 above).
2. `set_task_context` now returns a guard type that resets the TLS
    rather than taking a closure, removing 2 frames (13 and 14 above).

We can also remove frame 18 by removing `poll_with_tls_context` in favor
of a `get_task_context` function which returns a guard, but that
requires adjusting the code generated for .await, so I've left that off
for now.
bors added a commit that referenced this pull request Nov 15, 2019
Rollup of 12 pull requests

Successful merges:

 - #65557 (rename Error::iter_chain() and remove Error::iter_sources())
 - #66013 (Avoid hashing the key twice in `get_query()`.)
 - #66306 (Remove cannot mutate statics in initializer of another static error)
 - #66338 (Update mdbook.)
 - #66388 (Do not ICE on recovery from unmet associated type bound obligation)
 - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`)
 - #66391 (Do not ICE in `if` without `else` in `async fn`)
 - #66398 (Remove some stack frames from `.async` calls)
 - #66410 (miri: helper methods for max values of machine's usize/isize)
 - #66418 (Link to tracking issue in HIR const-check error code description)
 - #66419 (Don't warn labels beginning with `_` on unused_labels lint)
 - #66428 (Cleanup unused function)

Failed merges:

r? @ghost
@bors bors merged commit 3fe7cfc into rust-lang:master Nov 15, 2019
@sfackler sfackler deleted the no-async-nesting branch November 16, 2019 18:34
@jakubadamw
Copy link
Contributor

@sfackler, was the visibility change to set_task_context from public to private intentional? There are a few crates out there relying on it (futures-async-stream and gen-stream, for example).

@sfackler
Copy link
Member Author

Yes, it was intentional. The TLS interface is not a public API and will be removed entirely in the future.

AerialX added a commit to AerialX/core-futures-tls that referenced this pull request Dec 6, 2019
bors bot added a commit to taiki-e/futures-async-stream that referenced this pull request Dec 10, 2019
31: Fix build failure on latest nightly r=taiki-e a=taiki-e

This fixes breakage due to rust-lang/rust#66398.

Fixes #30 

Co-authored-by: Taiki Endo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants