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 CTFE stress test #266

Merged
merged 2 commits into from
Aug 9, 2018
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 3, 2018

Benchmark functions proposed by @oli-obk. I picked the iterations such that the crate compiles in 30s with rust-lang/rust#52925 landed.

Given that changing benchmarks later is not a good idea -- are there other things we should add?

@Mark-Simulacrum
Copy link
Member

This looks good to me, but let's get @oli-obk's eyes on it too to make sure we're testing the right thing.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2018

I'm very positive this does the right thing. Even considering the overhead of all the function calls that are going to be interpreted, we should get very good results from this. Even small changes to non-function call related things will get some measurable effect.

I don't know too much about benchmarking though. Since we have 9 different things we test, each test is about 3 seconds. Maybe we should expand on that? Like make the entire suite take 5 minutes, so each test gets around 30s?

Wrt future changes to benchmarks not being a good idea, I guess we can just add entirely new test crates for future things, or we make PRs that only change the benchmark (maybe even rename it) to create a structured break in the graphs

@Mark-Simulacrum
Copy link
Member

I'm fine with splitting this up.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2018

I'm fine with splitting this up.

Oh that would make tracking down the problem much simpler, too.

Can we add multiple bin targets to a benchmark crate and have each show up as a separate benchmark? They all share the explosion macro

@Mark-Simulacrum
Copy link
Member

Not today, but duplicating it doesn't seem too bad.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 3, 2018

Duplicating, as in, making this 9 or so separate crates?

I could share the macro with a symlink...

@Mark-Simulacrum
Copy link
Member

Symlinks might not work well because we copy the directories into /tmp which might be on a different file system during benchmarking. I think just copying the macro might be good enough.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2018

Maybe we can just do path dependencies and put the macro only in a single crate?

@Mark-Simulacrum
Copy link
Member

No, path dependencies (also) won't work because we copy the crates into a tempdir one by one. I would just duplicate the code, a more correct solution really isn't needed here.

@nnethercote
Copy link
Contributor

Like make the entire suite take 5 minutes, so each test gets around 30s?

That sounds excessive; it would make local runs significantly slower. And generally the results you get from a smaller stress test are representative of what you'd get from a larger stress test. tuple-stress may be a good comparison point for appropriate running time -- it was cut down from a much larger file in a real program so that it would not run too long.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2018

That sounds excessive; it would make local runs significantly slower. And generally the results you get from a smaller stress test are representative of what you'd get from a larger stress test. tuple-stress may be a good comparison point for appropriate running time -- it was cut down from a much larger file in a real program so that it would not run too long.

Larger tests help reduce statistical noise, though.

tuple-stress takes <5s on my system. I think I am going to try and target 10s for these tests.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2018

I updated this to one crate per test.

However, I also removed two tests because bumping them to take ~10s revealed that there is still some kind of regression likely introduced by the sanity_check PR, see rust-lang/rust#52849 (comment) for details.

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.

4 participants