PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.98k stars 4.62k forks source link

Add benchmarks to PCL #3860

Open kunaltyagi opened 4 years ago

kunaltyagi commented 4 years ago

Is your feature request related to a problem? Please describe.

Benchmarks will allow developers to test their change and iterate faster simply by recompiling (and saving the binaries) and running the benchmark tests.

Context

PCL has no bench-marking, which allows some regressions to slip through.

Describe the solution you'd like

chaytanyasinha commented 4 years ago

@kunaltyagi I have started working on benchmark for PCL as I have created a seperate benchmark folder and went through https://github.com/taketwo/pcl-transforms/blob/master/bench/bench_transforms.cpp

chaytanyasinha commented 4 years ago

@kunaltyagi I have found the solution for that error I mentioned in discord channel.

shrijitsingh99 commented 4 years ago

I think using google benchmark would be a good fit since we already use google tests. Any reason not to use google benchmark?

kunaltyagi commented 4 years ago

Both attack the issue from 2 ideas.

Google benchmark seems to be more flexible and well documented. Perhaps @taketwo can tell why he used Celero over Google Benchmark.

chaytanyasinha commented 4 years ago

@kunaltyagi I have started working on benchmark for PCL using google benchmark. Should I continue with that only?

kunaltyagi commented 4 years ago

It's not difficult to shift from one to another framework. Please continue using what you're using (and hopefully start a PR when PoC is ready to start integration)

chaytanyasinha commented 4 years ago

Sure @kunaltyagi

mvieth commented 4 years ago

Hi everyone, I started PR #4506 with some (very early) ideas for PCL benchmarks. Feel free to add comments. Some more ideas/considerations about benchmarks from me:

  1. I think the main point for benchmarks would be to check if a PR changes the performance of a function (faster or slower). Ideal would be a system that automatically creates an alert if something changed significantly, like "Congrats, function A only takes half the time than before". Another point would be to compare different functions that are trying to do similar things, but possibly at different speeds, e.g. RANSAC, RRANSAC, MSAC, RMSAC, ... in the sample consensus module. The results could be added to the documentation to help the user choose the best function.
  2. All the tests and checks on PRs are already taking a long time, additionally performing benchmarks should not make it take even longer. For some PRs, it would make sense to not do any benchmarking at all, e.g. typo fixes. It would be ideal if only those benchmarks would be performed where a speed change is possible. Alternatively, the benchmarks could be triggered, maybe by some keywords in the PR.
  3. To compute the speed change of a PR, there has to be a reference point (how fast are the functions in the current master?). google benchmarks can export to json and csv files, but those files have to be saved somewhere.
  4. I am not sure how reliable the benchmarks can be. I assume that the machines where the PR tests/checks are executed can change from run to run, so the timings might change just because they are done on another processor
  5. As far as I see, every function that we want to test needs its own benchmark, so it would be quite some work to add them
kunaltyagi commented 4 years ago

Thanks @mvieth for the summary.

  1. I like the idea of adding the comparison to the documentation itself somehow
  2. There's a way to use labels + github API to pick and choose the CI. Using labels like CI: typo and CI: benchmark would be a pretty slick way for the maintainers to opt-out of all and opt-into the more expensive benchmark CIs
  3. We can use another repository (similar to https://github.com/PointCloudLibrary/documentation) to do this. Baseline can be the latest release version
  4. This is a valid concern. A small sign of relief is that most of the VMs allocated by Azure for CI are quite standardized. When there's a HW update, then we'll see an unexplained boost in the benchmark
  5. Any ideas on how to reduce this burden?
mvieth commented 3 years ago
  1. That's cool! But then these labels would have to be added to the PRs right away when they are created, no?
  2. Good idea. I guess .ci/azure-pipelines/documentation.yaml could be adapted for that. But I think the baseline should be computed more often than releases, maybe weekly (like sunday midnight build)
  3. Not really. At least most benchmarks are rather straightforward to implement, or can even be copied from the tutorials
kunaltyagi commented 3 years ago
  1. We can add them afterwards and trigger a rerun by a force push 😆 (actually, just a rerun would be ok, since the check would happen in the CI)
  2. I was thinking of a release benchmark, and a running HEAD benchmark on master. The PR would run the ci test on demand to give a comparison