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 a stress test for uninit representations #507

Merged
merged 5 commits into from
Oct 16, 2019

Conversation

HeroicKatora
Copy link
Contributor

Test ctfe that never initializes a large portion of its value representations. This might be optimized independent of other changes to ctfe. The potential usefulness of having these benchmarks to evaluate such changes was discussed in rust-lang/rust#62655

cc: @oli-obk @RalfJung @Mark-Simulacrum

@HeroicKatora
Copy link
Contributor Author

HeroicKatora commented Oct 14, 2019

Addressed both of the review comments.

@Mark-Simulacrum
Copy link
Member

Will merge with CI passing.

@RalfJung
Copy link
Member

@HeroicKatora how long does that test take to run on your system (and how long does ctfe-stress-2 take)?

@Mark-Simulacrum
Copy link
Member

Hm, just copy/pasting the source code leads to a few compilation failures; looks like CI isn't catching it but https://travis-ci.com/rust-lang-nursery/rustc-perf/jobs/245574193#L1228 is a sample log

@HeroicKatora
Copy link
Contributor Author

HeroicKatora commented Oct 14, 2019

Turns out I am bad a copy&paste, the tests were mostly built from perf files I had flying around for my own testing and I didn't properly validated. Thanks CI.

After the import fix, current exection was ~4 seconds, so I've reduce the shift/size by 1 which brings it very close to a 2 seconds mark according to a comment in the other threads (clean build on nightly).

@Mark-Simulacrum
Copy link
Member

Yes, the specific execution time is not really relevant (due to differences in CPU speed etc); you probably want to be somewhere around the time that ctfe-stress-2 takes to execute locally for you.

@HeroicKatora
Copy link
Contributor Author

HeroicKatora commented Oct 14, 2019

ctfe-stress-2 clean definitely takes longer, about a factor of 5-6. Should I bump it up to that time or is faster also okay? I'm not quite sure how incremental would behave since ctfe-stress-uninit actually creates static variables while ctfe-stress-2 does not.

@RalfJung
Copy link
Member

you probably want to be somewhere around the time that ctfe-stress-2 takes to execute locally for you.

Wait what? No. This is like one of the tests in ctfe-stress-2, which are all geared to around 2s on my system (or rather they were when I originally submitted it).
There's like 10-15 tests in there, so your new test should be around 10-15 times faster than that.

That said, are these 4s with or without your PR? If that PR is going to cut the time down in half it may be worth factoring that in already.

@Mark-Simulacrum
Copy link
Member

Ah, right, I forget that we had that many tests in there. In that case, yes; we will want just 1/10 or so. We can use timings from the PR rather than nightly, though.

@HeroicKatora
Copy link
Contributor Author

HeroicKatora commented Oct 14, 2019

There's like 10-15 tests in there, so your new test should be around 10-15 times faster than that.

That said, are these 4s with or without your PR? If that PR is going to cut the time down in half it may be worth factoring that in already.

This perf contains more than one case also, some that will be directly affected by the PR. Some other cases are not yet fully affected and included to ensure that future improvements don't need to add their own stress test crates, I'd say the ratio of directly affected code is about 50/50. SURROUND, NONE, SOME and all other cases that contain only some uninitialize memory are only partially improved. The PR improves the complete set of tests by about ~12% in the current state.

@HeroicKatora
Copy link
Contributor Author

With the PR, the timing difference between ctfe-stress-2 and ctfe-stress-uninit is locally a factor of 7.2 (13.6s vs. 1.9s).

@nnethercote
Copy link
Contributor

Can we just add this new code to ctfe-stress-2 and rename it as ctfe-stress-3? We previously had a bunch of separate CTFE benchmarks but merged them because they were bloating the output.

@HeroicKatora
Copy link
Contributor Author

HeroicKatora commented Oct 15, 2019

Which of the two options should I go with, then? Any differing opinions?

@Mark-Simulacrum
Copy link
Member

I think in this case -3 is probably the best option as it sidesteps most of the concerns and is presumably fairly easily.

@RalfJung
Copy link
Member

Sounds good to me.

@HeroicKatora
Copy link
Contributor Author

Sure, easy enough.

@@ -68,6 +68,8 @@ programs.
caused [poor performance](https://github.com/rust-lang/rust/issues/32278) in
the past.
- **ctfe-stress-2**: A stress test for compile-time function evaluation.
- **ctfe-stress-uninit**: A stress test for representation of values with
uninitialized bytes in compile-time function evaluation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it or merge the two comments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no uninit benchmark anymore, I would expect a single entry with -3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably just dropping the uninit bit; that can be moved to a comment in the file.

@Mark-Simulacrum Mark-Simulacrum merged commit ab80d14 into rust-lang:master Oct 16, 2019
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