-
Notifications
You must be signed in to change notification settings - Fork 153
add deeply-nested-async test #768
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
Conversation
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? |
I am not even joking |
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 |
A little note about the author of the test would have been appreciated :3 |
Yay, thanks 😄 |
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... |
hmm, I think Would it make sense to keep both of them for a short while and then remove |
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. |
r? @Mark-Simulacrum
cc @tmandry