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

Make warmup_time be configurable #1844

Merged
merged 1 commit into from
Feb 1, 2020
Merged

Make warmup_time be configurable #1844

merged 1 commit into from
Feb 1, 2020

Conversation

khyperia
Copy link
Contributor

There's a constant, warmup_time, that is used for both timer statistics discovery, as well as test warmup. The timer statistics usage is fine, but, the default constant of 100ms used for each and every test warmup makes running a large number of benchmarks take an unreasonable amount of time (the project I'm working on has many very small/fast benchmarks).

This PR makes that constant be configurable with a command line switch, instead of a const.

Thanks! ✨

@@ -213,6 +213,9 @@ namespace Catch {
| Opt( config.benchmarkNoAnalysis )
["--benchmark-no-analysis"]
( "perform only measurements; do not perform any analysis" )
| Opt( config.benchmarkWarmupTime, "benchmarkWarmupTime" )
["--benchmark-warmup-time"]
( "amount of time in milliseconds spent on warming up each test (default: 500)" )
Copy link
Member

Choose a reason for hiding this comment

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

This does not track with the default in code.

<pre>--benchmark-warmup-time</pre>

Configure the amount of time spent warming up each test.

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 get an "introduced in" placeholder.

@horenmar
Copy link
Member

I'll have proper look later this week, but two things jumped out to me.

@horenmar horenmar added the Tweak label Jan 28, 2020
@codecov
Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #1844 into master will decrease coverage by 0.03%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
- Coverage   86.27%   86.23%   -0.03%     
==========================================
  Files         131      131              
  Lines        3888     3893       +5     
==========================================
+ Hits         3354     3357       +3     
- Misses        534      536       +2

@khyperia
Copy link
Contributor Author

Great! Also, the constant warmup_time is still used in the timer resolution/cost calculation warmup, let me know if you think that should also use this flag, or if it should use the warmup_time constant. (I have a local change that makes it also use the flag, but, yeah)

@horenmar
Copy link
Member

horenmar commented Feb 1, 2020

IIRC those are done only once, so they shouldn't cause a problem for users with a large numbers of benchmarks.

Anyway, I fixed a missing ToC entry in docs and rebased the approvals, I'll merge it after CI passes.

@khyperia
Copy link
Contributor Author

khyperia commented Feb 1, 2020

thanks so much! ❤️

@horenmar horenmar merged commit ccb1f70 into catchorg:master Feb 1, 2020
@khyperia khyperia deleted the benchmarkWarmupTime branch February 26, 2020 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants