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

add deeply-nested-async test #768

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 23, 2020

@Mark-Simulacrum
Copy link
Member

How long does this currently take for you to run locally? We need it to run in roughly one second (at least after any potential patches in flight).

Is this sufficiently different from the await call tree test we already have?

@lcnr
Copy link
Contributor Author

lcnr commented Sep 23, 2020

lcnr@lcnr-pc:~/wtf$ cargo +feca2c229e209ceda1eda7306e6ea9ee0aaa9305 build
   Compiling wtf v0.1.0 (/home/lcnr/wtf)
    Finished dev [unoptimized + debuginfo] target(s) in 1.00s

I am not even joking

@lcnr
Copy link
Contributor Author

lcnr commented Sep 23, 2020

The current await call tree test did not catch that regression.

rust-lang/rust#76928 improves the performance of this test from 3.5s to 1.0s while the current benchmarks were all mostly neutral, so I think this is sufficiently different to other benchmarks

@kellerkindt
Copy link

A little note about the author of the test would have been appreciated :3

@kellerkindt
Copy link

Yay, thanks 😄

@Mark-Simulacrum
Copy link
Member

Thanks for adding docs on deeply-nested-closures as well. It might be good to try and understand why await-call-tree does not catch this. Maybe this new benchmark is just universally better and we should drop await-call-tree? (2 "micro" benchmarks testing the same thing is not great; it adds overall suite runtime and confuses our results).

I think the main difference is that this has arguments to the async functions and the other one didn't...

@lcnr
Copy link
Contributor Author

lcnr commented Sep 23, 2020

hmm, I think deeply-nested-async covers strictly more than await-call-tree but I am not completely sure about that.

Would it make sense to keep both of them for a short while and then remove await-call-tree if we don't catch any regressions/improvements which only affect one of these tests?

@Mark-Simulacrum
Copy link
Member

I'm sure we'll be waiting a long time before we realize that we've not seen anything :)

I'll merge for now though.

@Mark-Simulacrum Mark-Simulacrum merged commit 0953d8f into rust-lang:master Sep 23, 2020
@lcnr lcnr deleted the add-nested-asny-test branch September 23, 2020 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants