Skip to content

[sample_consensus] Optimize Ellipse3D build performance#6428

Open
AlrIsmail wants to merge 1 commit intoPointCloudLibrary:masterfrom
AlrIsmail:optimize-ellipse3d-build
Open

[sample_consensus] Optimize Ellipse3D build performance#6428
AlrIsmail wants to merge 1 commit intoPointCloudLibrary:masterfrom
AlrIsmail:optimize-ellipse3d-build

Conversation

@AlrIsmail
Copy link
Copy Markdown

This pull request refactors and modularizes the 3D ellipse sample consensus model implementation in PCL. The main changes involve moving previously private or static methods and structures from the SampleConsensusModelEllipse3D class into a new pcl::internal namespace, improving code organization, testability, and separation of concerns. Additionally, key geometric and optimization routines are now defined and documented as standalone functions, and template instantiation is updated for clarity. (#3414)

Impact

  • Parsing overhead: Reduced from 11.9s to 0.6s (95% gain).
  • Total build time: 44% improvement.
  • Binary size: 12% reduction in object file size.

Changes

  • De-templated fitting math and LM optimizer to reduce frontend bloat.
  • Improved numerical stability via axis normalization and double-precision solvers.

@AlrIsmail
Copy link
Copy Markdown
Author

I noticed that sac_model_ellipse3d was really slowing down the build, so I tried to optimize it by de-templating the math kernels (inspired by what's already being done in the Cylinder and Cone modules). I'd love to hear what you think of the approach

@AlrIsmail AlrIsmail force-pushed the optimize-ellipse3d-build branch from b8e5a16 to 8d85945 Compare April 18, 2026 12:26
Refactor heavy mathematical kernels into non-templated pcl::internal functions.
Reduces header parsing time by ~95% and module build time by ~44%.
Ensures axis stability through explicit normalization during refinement.
Includes PCL_EXPORTS for full Windows DLL compatibility.
Follows PCL hybrid pattern and style guide.
@AlrIsmail AlrIsmail force-pushed the optimize-ellipse3d-build branch from 638e80e to c40a8b9 Compare April 18, 2026 14:46
@mvieth
Copy link
Copy Markdown
Member

mvieth commented Apr 19, 2026

Thank you for your pull request. I agree that there are opportunities to optimize in the ellipse3D model, however you put several independent changes into this pull request (computeModelCoefficientsEllipse3D, getDistancesToModelEllipse3D, optimizeModelCoefficientsEllipse3D, ...), which makes it very difficult to judge, which changes actually reduce the compile time? Which changes do nothing, or even make compile time worse? Additionally, we have to analyse, how the changes affect run time. We do not want to make a change that decreases compile time but increases run time.

For reference, here is the build time analysis of only sac_model_ellipse3d.cpp:
ellipse3d_build_analysis.txt

You can see that Eigen::LevenbergMarquardt is instantiated 18 times (once for each point type), so I would tend to agree that introducing pcl::internal::optimizeModelCoefficientsEllipse3D and calling it in optimizeModelCoefficients makes sense (like I did for the cylinder model, for example). Could you move that change to a separate pull request, please?

On the other hand, Eigen::GeneralizedEigenSolver for example is instantiated only once, so I am not convinced that moving code from computeModelCoefficients to internal::computeModelCoefficientsEllipse3D is actually helpful.

And for getDistancesToModelEllipse3D, I am wondering if this might even make the code run slower because all the xyz have to be copied?

So in conclusion, I would ask you to analyse each independent code change (computeModelCoefficientsEllipse3D, getDistancesToModelEllipse3D, optimizeModelCoefficientsEllipse3D, ...) separately, check if it actually decreases compile time, check if it does not increase run time (with a quick benchmark, see the benchmarks folder), then if you are still convinced that the specific code change is helping, create a separate pull request for it, so we can discuss and merge it individually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants