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 await-call-tree benchmark #508

Merged
merged 1 commit into from Oct 15, 2019

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Oct 14, 2019

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

@Mark-Simulacrum
Copy link
Member

Can you see that this has comparable instruction count / runtime with some of the other benchmarks? In particular e.g. deep-vector is probably a good comparison point.

@tmandry
Copy link
Member Author

tmandry commented Oct 14, 2019

Can you see that this has comparable instruction count / runtime with some of the other benchmarks? In particular e.g. deep-vector is probably a good comparison point.

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.

@Mark-Simulacrum
Copy link
Member

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.

@tmandry
Copy link
Member Author

tmandry commented Oct 15, 2019

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.
For debug builds it's 25 seconds before, 11 seconds after.
Is that too long?

@tmandry
Copy link
Member Author

tmandry commented Oct 15, 2019

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.

@Mark-Simulacrum
Copy link
Member

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 :)

@Mark-Simulacrum
Copy link
Member

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.

This sounds perfect, after fix is pretty much all that matters here I think.

I'll try to get this merged tomorrow.

@tmandry
Copy link
Member Author

tmandry commented Oct 15, 2019

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?

@Mark-Simulacrum
Copy link
Member

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 :)

@tmandry
Copy link
Member Author

tmandry commented Oct 15, 2019

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.
Copy link
Contributor

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!

Copy link
Member Author

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 :)

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.

check_mod_item_types query times regressed bigly
3 participants