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" #68811

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Feb 3, 2020

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2020
@tmandry
Copy link
Member Author

tmandry commented Feb 3, 2020

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 3, 2020

⌛ Trying commit c27d6f5 with merge 976ac3b...

bors added a commit that referenced this pull request 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
@bors
Copy link
Contributor

bors commented Feb 4, 2020

☀️ Try build successful - checks-azure
Build commit: 976ac3b (976ac3b5cd579bc2040b7473df7d885790a47082)

@rust-timer
Copy link
Collaborator

Queued 976ac3b with parent 8417d68, future comparison URL.

@Arnavion
Copy link

Arnavion commented Feb 4, 2020

In case you haven't already, please also ensure #67757 doesn't get reintroduced. Thanks.

@tmandry
Copy link
Member Author

tmandry commented Feb 4, 2020

@Arnavion Thanks for pointing to that, I'd indeed forgotten about the type length limit problem. I'm not quite sure what to do about it. We discussed this during the original PR (around here) and thought we had a fix for it in #65743, but that seems to have only been a partial fix. cc @eddyb

The perf run seems to be done; results are not great for async benchmarks, but not a total disaster either. (This is far better than the original regression which led us to revert the change.) While it might be possible to land the change with this regression, I'd prefer to dig into it some more first.

So, in summary, I think we need a fix to #67757, and to dig into the remaining performance regression. If anyone wants to take on either of these (or both), please feel free!

eddyb
eddyb reviewed Feb 5, 2020
View reviewed changes
src/librustc_ast_lowering/expr.rs
@@ -521,7 +521,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

/// Desugar `<expr>.await` into:
/// ```rust
/// match <expr> {
/// match ::std::future::IntoFuture::into_future(<expr>) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how #67757 is possible, the only HIR difference here should be handled by #65743.

@JohnCSimon JohnCSimon 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 Feb 16, 2020
@jonas-schievink jonas-schievink mentioned this pull request Feb 25, 2020
Closed
@Aaron1011
Copy link
Member

I think #69218 should help improve performance here - this PR appears to introduce many additional predicates during typeck, which end up getting stalled for several select iterations.

@danieleades danieleades mentioned this pull request Mar 7, 2020
Open
@joelpalmer
Copy link

Triaged

@Dylan-DPC-zz
Copy link

@tmandry any updates?

@Dylan-DPC-zz
Copy link

Closing this due to inactivity

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

Assignees

@cramertj cramertj

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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