Benchmarks auto repetitions

[MarcelKoch]

This PR enables the automatic deduction of repetition number for the benchmarks.

For small working sets, the benchmark timings may be too sensitive regarding outliers. With this PR the number of repetitions for the benchmark run is estimated such that the whole benchmark takes >=0.5s. This should result in more stable benchmarks for small problems.

If the repetitions are set to auto the warm-up step is skipped.

WIP: The PR enables the new behavior only for the blas, conversion, spmv benchmarks.
Todo:

• enable for solver, preconditioner bencharks
• handle large benchmarks (one repetition takes >0.5s) gracefully

[yhmtsai]

do we need to estimate the timing or just use warmup iteration + do repetition until reach the 0.5 sec?
does the warmup (estimate) needs to take 0.25 sec to warmup the device?

[Slaedr]

What's the reasoning for taking total time as the criterion? With statistics in mind, perhaps it is the number of repetitions that's more important. I like total time for other considerations, such as making sure enough samples are collected by a profiler, but that does not seem to be applicable here.

[MarcelKoch]

@yhmtsai The repetitions are estimated before the real benchmark run. This way, the timings within the rep loop might be moved outside to reduce the overhead in that loop.
The estimate runs are on the same device as the real benchmark run.

[MarcelKoch]

@Slaedr In my experience, the runtime for small problems (within L1/L2 cache) can vary quite significantly if only a low number of repetitions is used. With this PR I want to get a more stable average runtime for such problems. I should add, that now the number of repetitions is also exported in the json output.

[upsj]

How about we start off with warmup + a fixed number of iterations, compute the standard deviation of the runtime in the fixed set, and if that exceeds a certain value relative to the average, we increase the number of iterations by some scheme? I'll check what quickbench, GBench and nonius are doing there.

[yhmtsai]

for blas, use the prepare for the estimate. Some prepare functions are empty. Is it acceptable?
for spmv, use the apply without refresh the memory.
If we would like to refresh the memory in each timing, we can not move the timing out of loop

[MarcelKoch]

Concerning the pre and post operations, I was also not sure if they are used correctly/necessary. I guess I will look a bit more into that.

[Slaedr]

@MarcelKoch Ah, perhaps I see your logic. The total time is a good rule-of-thumb for estimating a good number of repetitions. If something takes very little time, we are likely to need many repeats, which your rule will do. If something takes a lot of time, perhaps it needs only a few repeats, which again is factored in your scheme. There might be some corner cases though, where some poorly-implemented algorithm takes a lot of time, but its runtime is still quite variable. I think what @upsj proposed might be more statistically sound; I remember doing that manually for some my studies in the past.

[MarcelKoch]

@Slaedr Yes exactly, the runtime is just used as a rule of thumb. I picked up 0.5s as a reasonable minimal runtime somewhere, although I'm not sure where exactly. That runtime can be discussed, if it's too high or too low.

@upsj Running these kinds of statistical tests seems a bit overkill to me. At least in my experience, the variation was quite low for these larger runtimes (>=0.5s), assuming that the machine does not use frequency scaling. If that is enabled, benchmarks can be quite unreliable, so I just ignored that case.
Also, starting with a fixed number of iterations might not be a good choice if one iteration already takes long. That case is also on my to-do list.

[upsj]

So nonius provides really powerful analysis, but also mainly analyzes tiny pieces of code, so the methods they use there (bootstrapping) don't make much sense on such small sample sizes.
So I would suggest using either the standard deviation or quartile distance as a measure for variability.

We have a certain variety of runtimes (going from fast to slow: overhead benchmarks (nanoseconds), BLAS, SpMV, preconditioners, solvers (seconds)), so I think it might make sense to have something robust that works on all of them. Requiring SpMVs run for 0.5s looks like a lot of overhead to me, especially since GPUs often have much less variability than CPUs.

[upsj]

So remembering my statistics lectures back in the day, I think if we want to reduce the standard deviation by a factor of 2, we need to run 4x as many benchmarks, so I guess my suggestion would be

1. compute rel_stddev = stddev / average
2. compute scale = rel_stddev / rel_stddev_limit
3. if scale > 1: run a factor of scale * scale - 1 additional iterations (limited by some max number of iterations)
4. if the standard deviation didn't decrease significantly then, stop anyways and report statistics (quantiles, outliers)

[tcojean]

I definitely agree with most of what is said here. I remember at the very beginning when we were considering using google benchmarks, the obscure algorithm they used and exactly this scheme of stopping after reaching 0.5s without extra control nor detailed information (i.e. vector of timings) was very weird, that creates an apple to oranges comparison in the timings, i.e. you compare qualitatively very different results depending on problem considered, problem size and other such things. Particularly, a lot of our solvers and other building blocks go very much past that time, so all of this is then not very useful.

IMHO, either you use a same amount of runs for matrices of comparable time scale (like what we do now), or you use an algorithm like what Tobias tried to outline which you apply equally all the time to reach the same timing accuracy all the time. At the risk of some benchmarks taking a ridiculous amount of time.

[tcojean]

On the cache effects issue for small problems, another probably more accurate approach is to have proper cache warmup and cache flushing strategies (depending on the context) to stabilize the timings, see this excellent paper on the topic. https://homes.sice.indiana.edu/rcwhaley/papers/timing_SPE08.pdf

Of course, there are still important potential performance issues which can come into play (particularly for large data sets), like process placement, turboboost and other speed scaling effects, ...

[MarcelKoch]

I guess I don't fully understand the purpose of these benchmarks. I interpreted them as a quick check to see if some changes significantly impact the performance in either way. (With this I mean that the purpose of the benchmarks is not to detect performance changes in the range 5%-1%)

For such a broad comparison, I think this approach is reasonable, both the runtime based and statistics based approach.
On the other hand, a more detailed analysis would require much more effort, as @tcojean already mentioned.

[MarcelKoch]

Concerning @upsj approach: I'm a bit unsure about the specifics of it. First, what would be a good threshold for the relative stddev? I would guess 1, but I've nothing to support that guess. This is also connected to the underlying distribution, where I'm also not sure which one to assume. The normal distribution would be the easiest choice, but again I've nothing to support that assumption.

[upsj]

I realized I was actually mixing up two things: stddev of the runtime distribution (which is fixed) and stddev of the mean estimator (which decays like sqrt(n) independent of the distribution). You can get the stddev of the mean estimator by bootstrapping, but that may be overkill here. I need to think about this some more.

As an example I ran a small BLAS benchmarks 1000 times and collected runtimes with 4 and 40 threads

[upsj]

I think we should be able to catch all our current cases (quick benchmarks, slow benchmarks) by providing a maximum repetition count used only for the repetition estimate. If the benchmark is slow, then we will choose the number of iterations based on the runtime. If it is fast, we will choose it so we have a sufficient number of runs (let's say 100), but still stay way below 0.5s
That is especially interesting if we are testing hundreds to thousands of small problems, where 0.5s per problem becomes significant. Also I would make both 0.5s and 100 configurable via command-line parameter.
At the moment, we don't seem to have benchmarks that require more sophisticated statistical treatment (though our current timer setup allows for it), but that may change when moving to MPI. That is probably a topic for another PR.

[upsj]

label!

[MarcelKoch]

I've incorporated a couple of suggestions from this thread.

Now there is more control over the adaptive benchmarking with the additional flags:

• min_repetitions
• max_repetitions
• min_runtime

Also, larger (or rather slower) benchmarks are now handled correctly, i.e. only the minimal requested number of repetitions is executed. Therefore, I've also enabled the adaptive behavior for the solver benchmark.

For the preconditioner, this approach is not possible, since the total runtime is not updated within the repetitions loop. Therefore, I've added a warning if -repetitions auto is used in that case and use the default number of repetitions as a fallback.

On the implementation side, the usage is quite similar to google benchmark, i.e. the following code is valid:

IterationControl ic(timer);
for(auto status: ic.run()){
  timer->tic()
  // run benchmark
  timer->toc()
}

Additionally, status may be used to check the number of the current iteration, and if it is the last iteration.

To clarify, the adaptive benchmarking is only optional and not enabled by default.

[codecov]

Codecov Report

Merging #791 (6b726f8) into develop (3112263) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 6b726f8 differs from pull request most recent head 33ff686. Consider uploading reports for the commit 33ff686 to get more accurate results

@@             Coverage Diff             @@
##           develop     #791      +/-   ##
===========================================
- Coverage    94.37%   94.36%   -0.01%     
===========================================
  Files          400      400              
  Lines        32096    32097       +1     
===========================================
  Hits         30289    30289              
- Misses        1807     1808       +1     

Impacted Files	Coverage Δ
omp/reorder/rcm_kernels.cpp	97.53% <0.00%> (-0.61%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3112263...33ff686. Read the comment docs.

[upsj]

LGTM, great job! I really like the Google Benchmark-like setup.

[yhmtsai]

this and HipTimer should not have the synchronize

[tcojean]

For the tic it doesn't matter right? This is not timed because it's before the eventRecord and it could even help in ensuring nothing is running on the GPU when we start running things, like if there was a copy previously (for example, x_clone).

[yhmtsai]

the event should also be after the memcpy if it involves gpu.
For me, ensuring nothing is running on the GPU is the stuff before calling timer, but it does not hurt the timing step.

[yhmtsai]

maybe we also add the tic/toc function to IterationControl to forward the Timer tic/toc.
such that we call tic/toc on the IC and get output from the same place as others?

[MarcelKoch]

First thing: you can already use ic.compute_average_timings to get the output, since ic has internally a copy of the shared ptr for the timer (but only for the apply timer). However, that seems a bit awkward.

From my viewpoint, IC should not handle any timings in this instance, as manage_timings is set to false. Adding timings functions to IC would weaken the distinction to the managed case. If the non-managed IC run is requested, the user should take care of the timings.

[yhmtsai]

It makes sense from this point.
My point is iteration control needs the results from the corresponding timer, so it can be wrong usage.
but I also realize that may make the tic/toc from TimerManager different?
maybe we need to add some comment about that need to use the corresponding timer such that the iteration control can really check it?

[MarcelKoch]

Perhaps I could add a get_timer to IC. That should help to use the correct timer. Otherwise, I can't think of a graceful way of adding tic/toc directly to IC.

[yhmtsai]

Yeah, it is good.

[yhmtsai]

this changes the behavior. we refreshed the memory every time before

[upsj]

does that matter? SpMVs should only write to x after all.

[yhmtsai]

no for result.
It depends on what we want the memory status for the benchmark.
Should memory always be a new location (only from software point), or the allocation just be there before the operations.

[upsj]

at least from the caching standpoint that would probably not make a difference, as the clone calls memcpy, which might, depending on the implementation, already move the data into cache.

[MarcelKoch]

I also think that this change should be fine, especially if you consider this as a best-case benchmark, i.e. the data is already in the appropriate caches. Considering the worst-case, i.e. at the beginning of each SpMV the data is not cached, is more difficult in general and would probably require more adjustments, especially wrt Tobias' comment.

[yhmtsai]

also here

[yhmtsai]

Suggested change

	"If 'repetitions = auto' is used, the minimal runtime of"
	"If 'repetitions = auto' is used, the minimal runtime (seconds) of"

[yhmtsai]

is the manage_timing also for warmuprun?

[MarcelKoch]

I have not added the parameter there, as the warmup-run uses always a fixed number of repetitions. I will clarify the documentation.

[yhmtsai]

Suggested change

	* @param timer the same timer that is to be used for the timings
	* @param timer the same timer that is to be used for the timings

[yhmtsai]

Suggested change

	status_warmup_.timer->clear();
	status_warmup_.timer->clear();
	status_warmup_.timer->manage_timings = false;

[yhmtsai]

I realize there's no change from another function, so it is fine.

[yhmtsai]

what is it used for?

[MarcelKoch]

That was used for being lazy 😄 I will remove that, and adjust the rest accordingly.

[yhmtsai]

Suggested change

	if (manage_timings) timer->tic();
	if (manage_timings) {
	timer->tic();
	}

also apply to next one from the gko ref

[MarcelKoch]

Is that part of the .clang-format specification? If not, perhaps it should be added there.

[yhmtsai]

it is not in current .clang-format. we only mention it in Contribution guideline.
Does clang-format support this after 6?

[MarcelKoch]

After a bit of digging, it seems like clang-format (up to 13) still does not support this. But there is an PR for this here: https://reviews.llvm.org/D95168
So it seems like that some future version will support this.

[MarcelKoch]

There is also a workaround using clang-tidy, but that is over kill (https://stackoverflow.com/a/28437960)

[tcojean]

LGTM!

[tcojean]

Maybe make the 1.5 controllable for extreme cases?

[yhmtsai]

LGTM. only two comments for missing documentation

[yhmtsai]

miss repetition_growth_factor

[yhmtsai]

not update

[yhmtsai]

nit

Suggested change

	* - 'repetitions_growth_factor' controls the increase between two successive
	* - 'repetition_growth_factor' controls the increase between two successive

or change the Gflags name

[yhmtsai]

need rebase before merge. and you should be able to see two pipelines running on gitlab after next push

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
17 Code Smells

0.0% Coverage
0.0% Duplication