Partial change from larger clipping performance work #1736

gunney1 · 2025-12-08T23:29:33Z

Summary

This PR is a feature and a refactoring. It's part of the planned follow-ups to the recently merged PR Initial clipping code re-factor #1667
It does the following:
- Implements screening in TetClipper to reduce use of expensive clipping method.
- Adds PlaneClipper, for clipping the 3D plane geometry. This is now the second geometry supported in the new clipping code.
- Factors out repetitive parts of MeshClipperImpl, addressing issue Re-factor MeshClipperImpl::computeClipVolumes3D #1704
- Switches to decomposing a hex into 18 tets instead of 24. Switching back and forth is currently supported by a compile-time parameter in ShapeMesh, for evaluating performance as we test against diverse sets of configuratios.
- Adds a flexible way to collect and log statistics, for evaluating performance. We can now add statistics without changing the core code.
- Add a developer parameter to control how much screening to use. In general, more screening is better, but there are overhead costs to screening.

Not included in this PR: shapes for the sphere, SOR (including cone and cylinder), hex, tet mesh. Further optimizations.

Performance

These are the number of clips used and the clipping time for the 2 shapes in this PR (tet and plane). They came from the same domain, with 3 mesh resolutions: 50^3, 100^3 and 200^3, respectively M50, M100 and M200. There are three levels of screening: L0 is no screening. L1 is screening on the hex cells. L2 is screening on the 18 individual tets in each hex. Values in the L1 and L2 columns are normalized by the L0 column for the same mesh, so they are ratios. Smaller mean more effective screening.

quantity	M50_L0	M50_L1	M50_L2	M100_L0	M100_L1	M100_L2	M200_L0	M200_L1	M200_L2
tetTime	0.15326	0.227	0.221	1.09257	0.137	0.14	7.986933	0.085	0.086
planeTime	0.379523	0.102	0.052	2.933372	0.054	0.03	22.81227	0.03	0.017
tetClips	975240.0	0.074	0.023	7711200.0	0.037	0.011	59687712.0	0.019	0.006
planeClips	2250000.0	0.035	0.009	18000000.0	0.017	0.005	144000000.0	0.009	0.002

Clipping times are in the first 2 rows; number clips are in the last 2 rows. Screening is more effective for the higher resolutions because the number of objects to be screened scale as resolution-cubed while the number of objects actually intersecting the geometry boundaries scale as resolution-squared. The data presented are for the test configuration. The plane cuts through the middle of the domain and the tet occupies about 1/16 of the domain volume. Remember that performance can be highly dependent on configuration, so this is not a thoroough test.

This WIP change to the mesh clipper code is meant to be more manageable for code review than the large change in the full change set. - Factor out repetitive parts of the code that computes volume contributions from primitive overlap. - Implements screening in TetClipper to improve performance. - Implements the Plane3DClipper, also with screening. - Switches to decomposing a hex into 18 tets instead of 24 tets. This should reduce the work load involving those tets. - Adds a general container for statics needed to evaluate clipping performance. - Adds parameter to control how much screening is done before the expensive clip loop. This is also for evaluating performance. - Fix at least 1 bug.

kennyweiss

Thanks @gunney1 -- nice speedups!

src/axom/core/utilities/Utilities.hpp

kennyweiss · 2025-12-11T21:54:58Z

src/axom/primal/tests/primal_sphere.cpp

+  using PointType = primal::Point<double, NDIMS>;
+  using SphereType = primal::Sphere<double, NDIMS>;
+
+  PointType center {0.0, 0.0, 0.0};


Please also test this on spheres that are not centered at the origin

kennyweiss · 2025-12-11T21:57:39Z

src/axom/quest/detail/clipping/Plane3DClipper.hpp

Please document the functions in this file.

@kennyweiss The public methods are implementations that should inherit documentation from the base class methods. Do you mean to explicitly add comments for them?

src/axom/primal/geometry/CoordinateTransformer.hpp

src/axom/quest/detail/clipping/MeshClipperImpl.hpp

src/axom/quest/detail/clipping/TetClipper.cpp

Arlie-Capps · 2025-12-16T23:16:27Z

src/axom/quest/examples/quest_shape_in_memory.cpp

+/************************************************************
+ * Shared variables.
+ ************************************************************/


This is an example, a tutorial on how to use our code. So perhaps the simplicity of using these global variables outweighs the uneasiness I feel. I'm still not convinced we need globals.

I get your uneasiness. The variables are not global, and I am adding static to them to make that clear.

static doesn't make global variables any better. If you absolutely need something like a global variable, such as for reference, put it in a singleton. We already have one singleton at line 342, Input params.

Arlie-Capps

I really appreciate your work on this, @gunney1 . I found a few typos: there may be more. I'm not convinced the shaping example should use the global variables. Please explain why that's the best way to structure the example, or rewrite it to be more modular. Beyond that, I approve.

Arlie-Capps

I realized my review was hasty. I'll spend some more time working on this.

For one thing, static or not, we should not have global variables in the code, even in examples.

Arlie-Capps · 2025-12-17T17:24:14Z

Brian, I appreciate your hard work, but you need to submit smaller pull requests. I suggest putting the sidre changes in one PR, everything else but the quest changes in a second PR, and then work on breaking the quest changes down further into a series of PRs. That will allow lots of things, including not burning out your reviewers.

rhornung67 · 2025-12-17T18:15:34Z

@Arlie-Capps I agree 100%. It's OK to slow down and be more careful and rigorous in development and reviews. Reasonably-sized, easily digestible PRs are an important piece of this.

gunney1 · 2025-12-18T00:13:07Z

As @Arlie-Capps suggested, I'll take out pieces of this PR to make small reviews. The sidre changes are in #1746

Arlie-Capps · 2025-12-23T16:25:23Z

src/axom/primal/geometry/Cone.hpp


  /*!
-   * \brief Returns the algebraic volume of the cone
+   * \brief Returns the algebraic volume of the cone,


Please write "signed volume" instead of "algebraic volume". We say "signed volume" elsewhere (such as in Tetrahedron).

Arlie-Capps · 2025-12-23T16:30:33Z

src/axom/sidre/core/Group.cpp

+  if(tupleAllocId == INVALID_ALLOCATOR_ID)
+  {
+    tupleAllocId = ConduitMemory::conduitAllocIdToAxom(dst.allocator());
+  }
+  if(arrayAllocId == INVALID_ALLOCATOR_ID)
+  {
+    arrayAllocId = ConduitMemory::conduitAllocIdToAxom(dst.allocator());
+  }


Why was this necessary? How does it interact with the check and adjustment at (new) line 1503, which seems to do the same thing (or very close)? Unless there's a very strong need, please remove either these lines or the check/adjustment at (new) line 1503. And if you do need both, please add a comment that clearly explains why.

gunney1 self-assigned this Dec 8, 2025

gunney1 added Quest Issues related to Axom's 'quest' component Performance Issues related to code performance labels Dec 8, 2025

gunney1 requested review from Arlie-Capps, BradWhitlock, jcs15c, kennyweiss and rhornung67 December 9, 2025 05:37

gunney1 marked this pull request as ready for review December 9, 2025 05:39

kennyweiss approved these changes Dec 11, 2025

View reviewed changes