[SPARK-15735] Allow specifying min time to run in microbenchmarks #13472
Conversation
| val timer = new Benchmark.Timer(i) | ||
| f(timer) | ||
| val runTime = timer.totalTime() | ||
| if (i > 0) { |
There was a problem hiding this comment.
This is a left-over from the previous version. Any idea why it was added? One warm-up period?
There was a problem hiding this comment.
I believe so. I would not be opposed to removing it.
|
Test build #59869 has finished for PR 13472 at commit
|
|
Test build #59898 has finished for PR 13472 at commit
|
|
In the interest of more reproducible results, added a warmup time and option to output to a file. cc @JoshRosen |
|
Test build #59957 has finished for PR 13472 at commit
|
|
Test build #59973 has finished for PR 13472 at commit
|
| defaultNumIters: Int = 5, | ||
| outputPerIteration: Boolean = false) { | ||
| minNumIters: Int = 2, | ||
| warmupTime: FiniteDuration = 2.seconds, |
There was a problem hiding this comment.
I like to add a warmup period, but is it better to 2 second as default. It depends on machine, program (we can specify a value explicitly), and others.
Are there any other thoughts?
There was a problem hiding this comment.
I suppose the relevant tradeoff here is accuracy vs time added to benchmark. I'd like to pick a value that is a reasonable default for most benchmarks - probably 2-5 seconds to warm up the jit reasonably depending on the program. Those with specialized needs can override it as needed.
|
Test build #60063 has finished for PR 13472 at commit
|
|
Test build #60061 has finished for PR 13472 at commit
|
|
This seems good to me. @hvanhovell, did you have any comments / do you want to do the final sign-off? |
|
LGTM |
|
Merging to master/2.0 |
## What changes were proposed in this pull request? This makes microbenchmarks run for at least 2 seconds by default, to allow some time for jit compilation to kick in. ## How was this patch tested? Tested manually with existing microbenchmarks. This change is backwards compatible in that existing microbenchmarks which specified numIters per-case will still run exactly that number of iterations. Microbenchmarks which previously overrode defaultNumIters now override minNumIters. cc hvanhovell Author: Eric Liang <[email protected]> Author: Eric Liang <[email protected]> Closes #13472 from ericl/spark-15735. (cherry picked from commit 4e8ac6e) Signed-off-by: Herman van Hovell <[email protected]>
## What changes were proposed in this pull request? This makes microbenchmarks run for at least 2 seconds by default, to allow some time for jit compilation to kick in. ## How was this patch tested? Tested manually with existing microbenchmarks. This change is backwards compatible in that existing microbenchmarks which specified numIters per-case will still run exactly that number of iterations. Microbenchmarks which previously overrode defaultNumIters now override minNumIters. cc hvanhovell Author: Eric Liang <[email protected]> Author: Eric Liang <[email protected]> Closes apache#13472 from ericl/spark-15735.
5 participants
What changes were proposed in this pull request?
This makes microbenchmarks run for at least 2 seconds by default, to allow some time for jit compilation to kick in.
How was this patch tested?
Tested manually with existing microbenchmarks. This change is backwards compatible in that existing microbenchmarks which specified numIters per-case will still run exactly that number of iterations. Microbenchmarks which previously overrode defaultNumIters now override minNumIters.
cc @hvanhovell