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.

Sign up for GitHub

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

Jump to bottom

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

Closed
tmandry opened this issue Jan 7, 2020 · 28 comments
Closed

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

tmandry opened this issue Jan 7, 2020 · 28 comments
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jan 7, 2020 •
edited by rustbot
Loading

#65244 was reverted in #67768 for performance reasons (see #67706). This issue tracks re-landing the support in a more performant way.

Tracking issue for IntoFuture trait: #67644

cc @seanmonstar @wesleywiser
@tmandry tmandry added the A-async-await Area: Async & Await label Jan 7, 2020
@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 7, 2020
@seanmonstar
Copy link
Contributor

Sorry for my lack of rustc internal knowledge, is there any hint or guess at why it was slower?

@tmandry
Copy link
Member Author

tmandry commented Jan 7, 2020

I don't think anyone knows for sure at this point. #67706 mentions normalize_ty_after_erasing_regions as the query that regressed.

cc @rust-lang/wg-traits

@JohnTitor JohnTitor added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 12, 2020
@tmandry tmandry added AsyncAwait-OnDeck AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. labels Jan 14, 2020
@tmandry
Copy link
Member Author

tmandry commented Jan 14, 2020

In the @rust-lang/wg-async-await meeting we agreed that this was a nice to have, but would be good to get it over the finish line. Tagging this as OnDeck, because we'd like to look into it while people still remember the implementation details.

@jonas-schievink
Copy link
Contributor

The same query regressed by 10% in #68524, also without being invoked more often.

@jonas-schievink jonas-schievink mentioned this issue Jan 28, 2020
Merged
bors added a commit that referenced this issue Jan 28, 2020
@bors
Auto merge of #68606 - jonas-schievink:normalize-fastpath, r=Zoxc
3761dcd 
Add an early-exit to `QueryNormalizer::fold_ty`

Pulled out from #68524, this results in a ~60-70% reduction in compile time for the await-call-tree benchmarks. This should unblock #67982 as well.

r? @Zoxc
@bstrie
Copy link
Contributor

bstrie commented Jan 31, 2020

#68606 alleges to unblock this issue, would anyone like to benchmark an un-reversion to see whether or not that's true?

@jonas-schievink
Copy link
Contributor

I made further perf improvements in #68672, so we can also wait until that lands.

@frondeus frondeus mentioned this issue Feb 1, 2020
Closed
@tmandry tmandry mentioned this issue Feb 3, 2020
Closed
bors added a commit that referenced this issue Feb 3, 2020
@bors
Auto merge of #68811 - tmandry:back-to-intofuture, r=<try>
976ac3b 
Re-land "add IntoFuture trait and support for await"

Testing the code from #65244 to see if the performance regressions are still there. #68606 and #68672 made perf optimizations that might interact with this change.

If this lands, fixes #67982.

cc @seanmonstar @jonas-schievink
r? @cramertj
@Xanewok
Copy link
Member

Xanewok commented Feb 18, 2020

The linked PR has been merged and it looks like the async/await benchmark received quite a nice boost.

Is it blocked on #68864 to make progress?

@nikomatsakis
Copy link
Contributor

I don't think it would be blocked on #68864, but maybe I'm missing something. Seems like we should re-open the PR and then maybe do some perf comparisons.

@jonas-schievink
Copy link
Contributor

It has been reopened as #68811. According to perf there's still a compile time regression of up to ~6% present.

@tmandry tmandry self-assigned this Mar 3, 2020
@seanmonstar seanmonstar mentioned this issue May 11, 2020
Open
@tmandry
Copy link
Member Author

tmandry commented May 19, 2020 •
edited
Loading

@rustbot assign @yoshuawuyts

Who was interested in getting at least the (unstable) trait merged. It'd be nice to do another perf run with the async/await integration, also.

@rustbot rustbot assigned rustbot and unassigned tmandry May 19, 2020
@Nemo157
Copy link
Member

Nemo157 commented Apr 26, 2021

#68811 (comment) seems to be the latest "what needs to be done next".

@ibraheemdev ibraheemdev mentioned this issue Aug 15, 2021
Closed
@yoshuawuyts yoshuawuyts mentioned this issue Nov 2, 2021
Closed
@eholk eholk mentioned this issue Nov 9, 2021
Merged
eholk added a commit to eholk/rust that referenced this issue Nov 22, 2021
@eholk
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
@danieleades
Copy link
Contributor

this seems to have stalled. that's a real shame, because I have a few crates where this would massively help with ergonomics

@Type1J
Copy link

Type1J commented Nov 29, 2021

@danieleades Same here.

@wesleywiser
Copy link
Member

There's work ongoing in #90737 to resolve this. Thanks @eholk and @yoshuawuyts! 🎉

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 3, 2021
@bors
Auto merge of rust-lang#90737 - eholk:intofuture, r=tmandry
532d2b1 
Reintroduce `into_future` in `.await` desugaring

This is a reintroduction of the remaining parts from rust-lang#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 rust-lang#67644, rust-lang#67982

/cc `@yoshuawuyts`
@camelid
Copy link
Member

camelid commented Dec 3, 2021

#90737 has been merged now. Should this issue be closed?

@eholk
Copy link
Contributor

eholk commented Dec 4, 2021

@camelid - Yes, the issue can be closed now. #67644 can probably be closed as well.

@camelid camelid closed this as completed Dec 4, 2021
@camelid
Copy link
Member

camelid commented Dec 4, 2021

#67644 can probably be closed as well.

AFAICT the feature hasn't been stabilized yet, so I think the tracking issue should stay open.

@eholk
Copy link
Contributor

eholk commented Dec 5, 2021

Sounds good to me!

@Type1J
Copy link

Type1J commented Dec 7, 2021

In Microsoft's windows crate, there are quite a few structs that implement a poll() function and the IntoFuture trait, but it only enabled IntoFuture on nightly. This is more their problem than Rust's, but if IntoFuture was in stable, it would most likely be fixed. Right now, I believe that they're in a "wait and see" mode.

@yoshuawuyts
Copy link
Member

@Type1J Do you have a link to the IntoFuture implementations in the windows crate? I found 4 Future trait implementations in the entire source, but no references to IntoFuture? Am I missing something?

I've recently started helping out on the windows-rs project, so if there is a dependency on IntoFuture from windows-rs I would love to know, as that would provide a convenient opportunity outside of the HTTP client space to validate how IntoFuture performs.

@Type1J
Copy link

Type1J commented Dec 9, 2021

My mistake. It seems to be a blanket implementation.

https://microsoft.github.io/windows-docs-rs/doc/windows/Foundation/struct.IAsyncOperation.html

@khuey
Copy link
Contributor

khuey commented Mar 28, 2022

This caused significant regressions (i.e. 25% or more) in stack frame size in debug builds of my application. Was that expected?

@tmandry
Copy link
Member Author

tmandry commented Mar 29, 2022

@khuey Fuchsia also saw significant regressions in stack frame size on debug builds; see #67644 (comment).

If you have any examples that are easy to reproduce, it would be much appreciated if you filed an issue. (Unfortunately, code in the Fuchsia tree is not easily built by outsiders.)

@khuey
Copy link
Contributor

khuey commented Mar 30, 2022

I could extract MIR or something but the code in question is not open source.

@eholk
Copy link
Contributor

eholk commented Mar 30, 2022

It's weird that we're only seeing these regressions on debug builds. I wouldn't expect the code around this to change between debug and release.

@khuey
Copy link
Contributor

khuey commented Mar 30, 2022

Presumably the optimizer is doing its job on release builds.

@tmandry tmandry moved this to Done in wg-async work Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
wg-async work
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Re-land "add IntoFuture trait and support for await" tmandry/rust