Reintroduce `into_future` in `.await` desugaring #90737

eholk · 2021-11-09T21:24:50Z

This is a reintroduction of the remaining parts from #65244 that have not been relanded yet.

This isn't quite ready to merge yet. The last attempt was reverting due to performance regressions, so we need to make sure this does not introduce those issues again.

Issues #67644, #67982

/cc @yoshuawuyts

rust-highfive · 2021-11-09T21:24:54Z

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

wesleywiser · 2021-11-09T21:57:21Z

@bors try @rust-timer queue

rust-timer · 2021-11-09T21:57:22Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-09T21:57:28Z

⌛ Trying commit 60f29450eeca029679562a3a0914975e5d8303a4 with merge 63f6208b5cec7496fd972de8245a4ff9d4da9e8b...

bors · 2021-11-09T23:32:17Z

☀️ Try build successful - checks-actions
Build commit: 63f6208b5cec7496fd972de8245a4ff9d4da9e8b (63f6208b5cec7496fd972de8245a4ff9d4da9e8b)

rust-timer · 2021-11-09T23:32:19Z

Queued 63f6208b5cec7496fd972de8245a4ff9d4da9e8b with parent d608229, future comparison URL.

rust-timer · 2021-11-10T02:17:31Z

Finished benchmarking commit (63f6208b5cec7496fd972de8245a4ff9d4da9e8b): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 9.4% on incr-full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

yoshuawuyts · 2021-11-10T09:33:37Z

Neat; we're doing a lot better than last time, where we had a 700% performance regression which basically made async/await unusable. This time around we're doing a lot better, and some amount of performance regression should probably be expected since we're actually doing more work.

I went in and verified that the output assembly is the same for both examples. Even though we're doing more work, we don't seem to be missing out on optimizing any of the generated code. [Future codegen | IntoFuture codegen | gist]

Personally I feel like this regression is small enough that we should prioritize landing this part of the async/.await RFC. It's been almost two years since we disabled IntoFuture as part of a short term fix, with little movement since. And comparing futures performance today to what it was back then, even with IntoFuture semantics enabled async/.await still compiles many times faster than it did in 2019¹.

How do other folks feel about this performance regression? Are there potentially any low-hanging performance optimizations we could apply to speed this up? If there are none, would folks be okay if we proposed to merge this sooner and optimize later?

If I understand the table right, -95% means "20x as fast". ↩

eholk · 2021-11-10T18:42:57Z

If I'm reading the detailed performance diff right, it looks like a lot of the extra time is being spent in evaluate_obligation and normalize_projection_ty. Given that this PR causes each await to introduce a new requirement for IntoFuture, this seems expected.

I'm not sure what the usual guidelines for accepting a regression are, but I'd be in favor of taking this one. Most of the benchmarks showed little change. The biggest regression was in deeply-nested-async, which is a pattern that probably doesn't come up too often (although the benchmark is based off a real-world scenario, so it's no unheard of). It would be nice to implement this part of the async/await RFC, even if that comes at a bit of a cost.

As far as ideas for reducing the regression, I think it'd be worth looking at the way generators get converted to futures. Maybe there's something that could be done that's symbiotic with this change and would lead to less work being done in total.

compiler/rustc_ast_lowering/src/expr.rs

michaelwoerister · 2021-11-11T08:00:04Z

Thanks for the PR, @eholk & @yoshuawuyts! This looks good to me. It would be nice to reduce the regression but it seems acceptable. The compiler just has to do more work so it is expected that compile time increase for the cases where this is exercised.

I am not particularly familiar with lowering (especially wrt async) so it would be good to have another pair of eyes on this:
r? @tmandry

tmandry · 2021-11-17T02:08:47Z

src/test/ui/async-await/async-fn-size-moved-locals.rs

-    assert_eq!(7181, std::mem::size_of_val(&mixed_sizes()));
+    assert_eq!(3076, std::mem::size_of_val(&joined()));
+    assert_eq!(3076, std::mem::size_of_val(&joined_with_noop()));
+    assert_eq!(6157, std::mem::size_of_val(&mixed_sizes()));


This is unexpected! Do you know why these sizes change?

It looks like an improvement but I'd like to understand what's going on.

We weren't sure either! The previous PR also shrunk the size of the generated futures, without really commenting on it either. It's particularly surprising because neither the numbers before or after this changed are aligned the way we expected them to.

We reasoned that this change would be alright since it doesn't regress, there is precedence for accepting this in #65244, and it doesn't lead to any other changes as far as we can tell. Even if we don't fully understand why the current results are what they are, or what is causing the change.

I don't know if this is right, but one guess is it has something to do with the match <future> { ... } becoming match InfoFuture::into_future(<future>) { ... }. Maybe the into_future call moves the future into the scrutinee position of the match, rather than borrowing from the outer scope?

Judging by the comment on this test, it seems like the first two changes aren't a big deal, since they are just a few bytes. The last change is surprising because it improves by exactly 1024. This means we were able to eliminate one of the BigFuts from the closure. By my count we should only need 5, so I'm not sure why we have 6 copies now or why we had 7 copies before. I wouldn't be surprised to see 8 or 5, I guess, but 6 and 7 are both weird. Could we be smarter about laying out the closures? Maybe there's a move from one slot to another that's forced by the way we laid out the closures?

Interesting theory, if we're turning a borrow into a move that can definitely improve things! See #62321 for instance – if I recall correctly, borrowing a local in a generator causes the analysis to assume the local is always live from that point, preventing it from reclaiming the storage.

src/test/ui/async-await/issue-70594.stderr

bors · 2021-11-21T10:45:20Z

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

This is a reintroduction of the remaining parts from rust-lang#65244 that have not been relanded yet. Issues rust-langGH-67644, rust-langGH-67982

tmandry

Looks good!

You can r=me after comments, also happy to take another look.

src/test/ui/async-await/await-into-future.rs

src/test/ui/async-await/unresolved_type_param.stderr

src/test/ui/async-await/issue-70594.stderr

Add a note about `IntoFuture` in error messages where T is not a future. Change await-into-future.rs to be a run-pass test.

eholk · 2021-12-02T19:47:23Z

Thanks for the suggestions. I believe they are all addressed, so feel free to take a look. I don't know if I have permission to do r=me, but I'll try it out to see.

@bors r=tmandry

eholk · 2021-12-03T17:45:54Z

@bors r=tmandry

bors · 2021-12-03T17:45:55Z

@eholk: 🔑 Insufficient privileges: Not in reviewers

tmandry · 2021-12-03T17:51:49Z

@bors r+

bors · 2021-12-03T17:51:50Z

📌 Commit 3268bf1 has been approved by tmandry

bors · 2021-12-03T19:29:29Z

⌛ Testing commit 3268bf1 with merge 532d2b1...

bors · 2021-12-03T22:36:57Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 532d2b1 to master...

rust-timer · 2021-12-04T00:07:25Z

Finished benchmarking commit (532d2b1): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 9.3% on incr-full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

eholk · 2021-12-04T00:19:46Z

@rustbot label: +perf-regression-triaged

We saw similar regressions during the earlier try job and decided it they were acceptable. See this earlier comment for some details. The summary is that the regressions are only in async heavy code and are expected because we are generating code that introduces more trait obligations. Overall, the performance is still much faster than the earlier attempt to land this change, and there are a lot of users requesting this feature, so it's worth taking the regression.

rust-highfive assigned michaelwoerister Nov 9, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 9, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 10, 2021

eholk marked this pull request as ready for review November 10, 2021 18:43

michaelwoerister reviewed Nov 11, 2021

View reviewed changes

compiler/rustc_ast_lowering/src/expr.rs Outdated Show resolved Hide resolved

rust-highfive assigned tmandry and unassigned michaelwoerister Nov 11, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021

rylev mentioned this pull request Nov 11, 2021

Operation Builder Future Azure/azure-sdk-for-rust#510

Merged

tmandry reviewed Nov 17, 2021

View reviewed changes

eholk added 3 commits November 22, 2021 14:57

Reintroduce into_future in .await desugaring

dfa0db5

This is a reintroduction of the remaining parts from rust-lang#65244 that have not been relanded yet. Issues rust-langGH-67644, rust-langGH-67982

Fix comments

ca2057c

Update stderr files

db80c07

eholk force-pushed the intofuture branch from 52a3a75 to db80c07 Compare November 22, 2021 23:59

wesleywiser mentioned this pull request Nov 29, 2021

Re-land "add IntoFuture trait and support for await" #67982

Closed

tmandry approved these changes Nov 30, 2021

View reviewed changes

src/test/ui/async-await/await-into-future.rs Outdated Show resolved Hide resolved

src/test/ui/async-await/unresolved_type_param.stderr Show resolved Hide resolved

src/test/ui/async-await/issue-70594.stderr Show resolved Hide resolved

Code review feedback

0cb7693

Add a note about `IntoFuture` in error messages where T is not a future. Change await-into-future.rs to be a run-pass test.

This comment has been minimized.

Sign in to view

Update .stderr files

3268bf1

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 Dec 3, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 3, 2021

bors merged commit 532d2b1 into rust-lang:master Dec 3, 2021

rustbot added this to the 1.59.0 milestone Dec 3, 2021

bors mentioned this pull request Dec 3, 2021

Tweak errors coming from for-loop, ? and .await desugaring #90939

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 4, 2021

WaffleLapkin mentioned this pull request Dec 5, 2021

Tracking issue for IntoFuture #67644

Closed

yoshuawuyts deleted the intofuture branch March 10, 2022 19:52

WaffleLapkin mentioned this pull request Jul 22, 2022

Feature Request: Remove AutoSend in favour of IntoFuture teloxide/teloxide#680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce `into_future` in `.await` desugaring #90737

Reintroduce `into_future` in `.await` desugaring #90737

eholk commented Nov 9, 2021

rust-highfive commented Nov 9, 2021

wesleywiser commented Nov 9, 2021

rust-timer commented Nov 9, 2021

bors commented Nov 9, 2021

bors commented Nov 9, 2021

rust-timer commented Nov 9, 2021

rust-timer commented Nov 10, 2021

yoshuawuyts commented Nov 10, 2021 •

edited

Loading

eholk commented Nov 10, 2021

michaelwoerister commented Nov 11, 2021

tmandry Nov 17, 2021

yoshuawuyts Nov 17, 2021

eholk Nov 23, 2021

tmandry Nov 30, 2021 •

edited

Loading

bors commented Nov 21, 2021

tmandry left a comment

eholk commented Dec 2, 2021 •

edited

Loading

This comment has been minimized.

eholk commented Dec 3, 2021

bors commented Dec 3, 2021

tmandry commented Dec 3, 2021

bors commented Dec 3, 2021

bors commented Dec 3, 2021

bors commented Dec 3, 2021

rust-timer commented Dec 4, 2021

eholk commented Dec 4, 2021

Reintroduce into_future in .await desugaring #90737

Reintroduce into_future in .await desugaring #90737

Conversation

eholk commented Nov 9, 2021

rust-highfive commented Nov 9, 2021

wesleywiser commented Nov 9, 2021

rust-timer commented Nov 9, 2021

bors commented Nov 9, 2021

bors commented Nov 9, 2021

rust-timer commented Nov 9, 2021

rust-timer commented Nov 10, 2021

yoshuawuyts commented Nov 10, 2021 • edited Loading

Footnotes

eholk commented Nov 10, 2021

michaelwoerister commented Nov 11, 2021

tmandry Nov 17, 2021

Choose a reason for hiding this comment

yoshuawuyts Nov 17, 2021

Choose a reason for hiding this comment

eholk Nov 23, 2021

Choose a reason for hiding this comment

tmandry Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Nov 21, 2021

tmandry left a comment

Choose a reason for hiding this comment

eholk commented Dec 2, 2021 • edited Loading

This comment has been minimized.

eholk commented Dec 3, 2021

bors commented Dec 3, 2021

tmandry commented Dec 3, 2021

bors commented Dec 3, 2021

bors commented Dec 3, 2021

bors commented Dec 3, 2021

rust-timer commented Dec 4, 2021

eholk commented Dec 4, 2021

Reintroduce `into_future` in `.await` desugaring #90737

Reintroduce `into_future` in `.await` desugaring #90737

yoshuawuyts commented Nov 10, 2021 •

edited

Loading

tmandry Nov 30, 2021 •

edited

Loading

eholk commented Dec 2, 2021 •

edited

Loading