Skip to content

Remove a stack frame from .await calls#70831

Merged
bors merged 2 commits into
rust-lang:masterfrom
sfackler:shrink-future-stack
Apr 16, 2020
Merged

Remove a stack frame from .await calls#70831
bors merged 2 commits into
rust-lang:masterfrom
sfackler:shrink-future-stack

Conversation

@sfackler

@sfackler sfackler commented Apr 6, 2020

Copy link
Copy Markdown
Member

The stack frames when .awaiting one async fn from another currently look like this:

  12: foo::b::{{closure}}
             at src/main.rs:2
  13: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /home/sfackler/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/future/mod.rs:66
  14: core::future::poll_with_context
             at /home/sfackler/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/future/mod.rs:84
  15: foo::a::{{closure}}
             at src/main.rs:6

Since the move away from using TLS to pass the Context around, it's now easy to remove frame 14 by removing poll_with_context in favor of calling Future::poll directly. This still leaves the GenFuture frame, but that seems significantly harder to deal with.

It also improves diagnostics a bit since they no longer talk about the private poll_with_context function.

@rust-highfive

rust-highfive commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

r? @matthewjasper

(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 Apr 6, 2020
@Centril

Centril commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

rust-timer commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@bors

bors commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 5dc8ec8 with merge c625e40f85b5b2f48f832564bf9dcdaeecc9a4fd...

@bors

bors commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-azure
Build commit: c625e40f85b5b2f48f832564bf9dcdaeecc9a4fd (c625e40f85b5b2f48f832564bf9dcdaeecc9a4fd)

@rust-timer

rust-timer commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

Queued c625e40f85b5b2f48f832564bf9dcdaeecc9a4fd with parent b543afc, future comparison URL.

@sfackler

sfackler commented Apr 9, 2020

Copy link
Copy Markdown
Member Author

It looks like the perf run finished but the bot didn't comment for some reason.

@Centril

Centril commented Apr 9, 2020

Copy link
Copy Markdown
Contributor

Yeah, that happens sometimes sadly.

matthewjasper
matthewjasper reviewed Apr 13, 2020
View reviewed changes

@matthewjasper matthewjasper left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with nit addressed if we think that the perf is good enough.

Comment thread src/librustc_span/symbol.rs Outdated
poll,
Poll,
poll_with_context,
get_context,

@matthewjasper matthewjasper Apr 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved up to be in order.

@sfackler sfackler Apr 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Centril Centril added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2020
@matthewjasper

matthewjasper commented Apr 16, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Apr 16, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 3ba3bd5 has been approved by matthewjasper

@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 Apr 16, 2020
@bors

bors commented Apr 16, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 3ba3bd5 with merge 4e4d49d...

@bors

bors commented Apr 16, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 4e4d49d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 16, 2020
@bors bors merged commit 4e4d49d into rust-lang:master Apr 16, 2020
@eddyb

This comment has been minimized.

Sign in to view
@sfackler sfackler deleted the shrink-future-stack branch April 16, 2020 22:23
@nikomatsakis nikomatsakis mentioned this pull request Apr 17, 2020
Open
@taiki-e taiki-e mentioned this pull request Apr 17, 2020
Merged
bors Bot added a commit to taiki-e/futures-async-stream that referenced this pull request Apr 17, 2020
@bors @taiki-e
Merge #37
86d6917 
37: Remove poll*with_context functions r=taiki-e a=taiki-e

Based on rust-lang/rust#70831

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@senden9 senden9 mentioned this pull request Apr 20, 2020
Closed
bors Bot added a commit to taiki-e/futures-async-stream that referenced this pull request May 2, 2020
@bors @taiki-e
Merge #37
153093f 
37: Remove poll*with_context functions r=taiki-e a=taiki-e

Based on rust-lang/rust#70831


Co-authored-by: Taiki Endo <te316e89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewjasper matthewjasper matthewjasper left review comments

Assignees

@matthewjasper matthewjasper

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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sfackler @rust-highfive @Centril @rust-timer @bors @matthewjasper @eddyb