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 benchmarking & some opti #227

Merged
merged 22 commits into from
May 24, 2021
Merged

Add benchmarking & some opti #227

merged 22 commits into from
May 24, 2021

Conversation

artivis
Copy link
Owner

@artivis artivis commented Apr 30, 2021

  • Add benchmark
  • some small opti
  • SE2Tangent::rjacinv/ljacinv
  • SE_2_3Tangent::rjacinv/ljacinv
  • fix average unit test
  • run tests in debug and release on ubuntu.

This PR adds some micro benchmarks using Google benchmark. This allows to have some time-based benchmarks on a per-function basis so that one can compare different implementations.

The PR also contains numerous small optimizations, most of which do not actually show any benefit in the aforementioned benchmark. Those which do are listed below.
Most noticeably, implementing SE2Tangent::rjacinv/ljacinv (~6x / ~4.3x) and SE_2_3Tangent::rjacinv/ljacinv (~4.5x / ~5x) are a big win.

It also remove some unnecessary cmake warning suppression for the test/examples, fix the average unit test that was disabled and finally it runs the unit test both in Debug and Release on Ubuntu.

Some numbers obtained through the benchmarking introduced in this PR:

Group Operation Before After
SE2 rjacinv
(mean) 49.6 ns 8.41 ns
(median) 49.6 ns 8.39 ns
(stddev) 3.69 ns 0.102 ns
ljacinv
37.1 ns 8.48 ns
37.3 ns 8.46 ns
3.77 ns 0.097 ns
SE_2_3 rjacinv
1570 ns 349 ns
1539 ns 354 ns
72.8 ns 9.76 ns
ljacinv
1722 ns 338 ns
1706 ns 337 ns
68.4 ns 11.0 ns
adj
100 ns 76.1 ns
100 ns 75.8 ns
1.11 ns 1.45 ns

As a bonus, here is compared the unit quaternion random implementation from Eigen and the one proposed in #105. Note that currently manif still uses the implementation from Eigen as I don't see a strong motivation to introduce some new code to support for a non critical function. Rather than changing manif implementation I'd suggest to propose the change directly upstream to Eigen.

Operation Before After
quaternion random
524 ns 491 ns
523 ns 490 ns
2.04 ns 1.99 ns

@artivis artivis self-assigned this Apr 30, 2021
@artivis artivis added the enhancement New feature or request label Apr 30, 2021
@artivis
Copy link
Owner Author

artivis commented Apr 30, 2021

Full disclaimer, I did not actually run the math for SE_2_3Tangent::rjacinv/ljacinv. I naively/straightforwardly applied the pattern I saw in SE3Tangent::rjacinv/ljacinv and the tests passed :)

@joansola
Copy link
Collaborator

In what consist the optimizations on ljacinv, rjacinv, etc?

@artivis
Copy link
Owner Author

artivis commented Apr 30, 2021

In what consist the optimizations on ljacinv, rjacinv, etc?

SE2Tangent::rjacinv/ljacinv and SE_2_3Tangent::rjacinv/ljacinv?
Well before they were implemented as the inverse of the respective Jacs (e.g. se2.rjac().inverse()) whereas this PR actually implements their analytical closed-form (straight to se2.rjacinv()).

@joansola
Copy link
Collaborator

Oh OK I see. Yep, obviously much faster!

@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #227 (ca61008) into devel (287c71e) will decrease coverage by 0.14%.
The diff coverage is 97.18%.

@@            Coverage Diff             @@
##            devel     #227      +/-   ##
==========================================
- Coverage   98.05%   97.90%   -0.15%     
==========================================
  Files          48       49       +1     
  Lines        1436     1531      +95     
==========================================
+ Hits         1408     1499      +91     
- Misses         28       32       +4     

@artivis artivis requested a review from joansola May 23, 2021 18:26
@artivis
Copy link
Owner Author

artivis commented May 23, 2021

@joansola This PR is ready when you are. Nevermind the failure coverage test nor the pending ones (looks like a bug).

Copy link
Collaborator

@joansola joansola left a comment

Choose a reason for hiding this comment

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

OK I already had a look to all this some time ago, I approve right away

@artivis artivis merged commit 5d1297b into devel May 24, 2021
@artivis artivis deleted the feature/benchmarking branch May 24, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants