-
Notifications
You must be signed in to change notification settings - Fork 148
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 await-call-tree benchmark #508
Conversation
Can you see that this has comparable instruction count / runtime with some of the other benchmarks? In particular e.g. |
A check run goes from 150B instructions before the fix to only 233 million instructions after, while deep-vector is about 5B instructions. A debug run goes from 256B to 107B instructions, while deep-vector is only about 8B. I'm currently running a few more for comparison, not sure if this answers your question or not. |
It would be good to lower the nesting such that this is about the same run time as that benchmark; this is too long for our use case probably. |
The check run time is only ~15 seconds before the fix; after it's about 0.52 seconds. |
Removing two levels of nesting gets us to <1s on debug builds (after fix) while showing a -94% reduction in instruction count on check, so going with that. |
dc65ac3
to
1f7f47e
Compare
Yes, most of our benchmarks are around 1-2 seconds long. 30 seconds is getting into script-servo territory, which is the longest benchmark today I believe; we run each benchmark 3 times per "type" of which there are 3 at minimum (clean, baseline incr, clean incr, see bottom of a compare page for more details); and then for each check/opt/debug -- all in all, that's 27 times at minimum pretty much so a 30 second benchmark would add ~15 minutes of runtime which is quite a lot :) |
This sounds perfect, after fix is pretty much all that matters here I think. I'll try to get this merged tomorrow. |
Great, thanks! Once it is merged can I kick off a rust-timer run immediately, or do I need to wait for the bot to be updated? |
No, I'll be re-deploying the collector pretty much immediately upon merging so you'll not need to wait at all (well, give me like ~5 minutes :) |
Perfect. Feel free to kick off a run on rust-lang/rust#65293 if I'm not around. |
- **await-call-tree**: A tree of 9 async fns that await each other, creating a | ||
large type composed of many repeated `impl Future` types. Such types caused | ||
[poor performance](https://github.com/rust-lang/rust/issues/65147) in the | ||
past. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for updating the docs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized the 9
is no longer correct, removing it :)
1f7f47e
to
ca5840a
Compare
To go with rust-lang/rust#65293 which fixes rust-lang/rust#65147. Locally I'm seeing a ~37x decrease in instruction count on check.
r? @Mark-Simulacrum