Skip to content

Route Boolean3::Result timing through the shared phase mechanism#1745

Open
zmerlynn wants to merge 2 commits into
elalish:masterfrom
zmerlynn:feat/boolean-result-phase-timing
Open

Route Boolean3::Result timing through the shared phase mechanism#1745
zmerlynn wants to merge 2 commits into
elalish:masterfrom
zmerlynn:feat/boolean-result-phase-timing

Conversation

@zmerlynn
Copy link
Copy Markdown
Contributor

@zmerlynn zmerlynn commented Jun 2, 2026

Follow-up to #1742, addressing the review ask: reuse the new per-phase timing to drop Boolean's hand-placed Timers, for consistency.

Result's four Timers (Assembly/Triangulation/Simplification/Sorting) bracketed spans that were already unions of phase() boundaries, so the phase() lambda now records through the same path as the ADVANCE_PHASE_OR_RETURN ops, via a new explicit-line MANIFOLD_RECORD_PHASE_AT (a lambda called from 11 sites would otherwise report a single line). BeginPhaseTiming seeds the baseline at Result entry, since Boolean never hits the static-factory reset that seeds it for LevelSet/FromMesh/Smooth. perfPhases now drives its booleans through an ExecutionContext so the lines print.

Every phased op now emits the one phase N (file:line) = X ms format. The intersection-stage Timers are kept; the 11 phase lines replace the old four named Timers:

=== Sphere(32): subtract ===
----------- 0 ms for Intersect12 P->Q
----------- 0 ms for Intersections (total)
  phase 1 (boolean_result.cpp:772) = 0.0002 ms
  ...
  phase 11 (boolean_result.cpp:959) = 3.38 ms
366 verts and 728 tris

Notes:

  • Kept the intersection-stage Timers in the Boolean3 ctor (Intersect12 P->Q / Q->P, Winding03 P / Q): a P/Q sub-op breakdown, not sequential phases.
  • The file:line keys drop the old "Assembly"/etc. labels.

Progress/cancel semantics and the release build are unchanged - timing only records with a ctx attached, under MANIFOLD_DEBUG/_TIMING.

Per Emmett's elalish#1742 review: drop the four hand-placed Result Timers
(Assembly/Triangulation/Simplification/Sorting) and report through the
same per-phase timing as LevelSet/FromMesh/Smooth, so all phased ops emit
the one `phase N (file:line) = X ms` format.

The phase() lambda already brackets those spans (the 4 named regions were
just unions of consecutive phase boundaries), so it gains a `line` arg and
records via a new MANIFOLD_RECORD_PHASE_AT - an explicit-line variant, since
a lambda called from 11 sites has only its own body line. Boolean never hits
the static-factory reset that seeds `lastPhase`, so BeginPhaseTiming stamps
the baseline at Result entry.

Left alone: the intersection-stage Timers in the Boolean3 ctor (a P/Q
sub-op breakdown, not sequential phases) and the verts/tris summary.
perfPhases now drives its booleans through an ExecutionContext so the phase
lines print. Progress/cancel semantics and the release build are unchanged
(timing only records with a ctx attached, under MANIFOLD_DEBUG/_TIMING).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

❌ Patch coverage is 73.33333% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.89%. Comparing base (37125da) to head (e458f21).

Files with missing lines Patch % Lines
src/execution_impl.cpp 57.14% 6 Missing ⚠️
src/boolean_result.cpp 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
- Coverage   94.93%   94.89%   -0.05%     
==========================================
  Files          37       37              
  Lines        8377     8377              
==========================================
- Hits         7953     7949       -4     
- Misses        424      428       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread extras/perf_phases.cpp
result.NumTri();
ExecutionContext ctx;
Manifold result = op.apply(a, b).WithContext(ctx);
result.Status(); // force evaluation under ctx so phases time and print
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What will happen if a and b are themselves the result of a tree of boolean operations and everything gets evaluated in parallel at Status? What will the timing prints look like?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch - it was a real bug. The timing baseline lived on the shared ExecutionContext, so parallel Result calls clobbered each other's deltas and the lines interleaved unattributably.

Fixed: the baseline is now a per-Result stack local, and each line carries a per-boolean uid. Still interleaved (eval is parallel), but each boolean's deltas are correct and splittable by tag:

  [b0] phase 7 = 0.026 ms
  [b1] phase 1 = 0.001 ms
  [b0] phase 8 = 0.228 ms   <- since b0's phase 7, not b1's
  [b1] phase 2 = 0.017 ms

grep '\[b0\]' recovers b0's clean 1..11. Static factories (single-threaded) are unchanged. Verified race-free under TSan.

Addresses the elalish#1745 review: the per-phase timing keyed off the shared
`ctx->lastPhase`, so when a CSG tree evaluates booleans concurrently the
parallel `Boolean3::Result` calls clobbered each other's baseline (garbage
deltas) and their phase lines interleaved with no way to tell them apart.

Move the baseline to a per-`Result` stack local (`LocalPhaseTiming`) and tag
every printed line with a per-boolean uid (`[bN]`) - so concurrent booleans
keep correct deltas and the interleaved output is splittable. The static
factories keep the ctx baseline (single-threaded, unaffected). Debug-only
(verbose >= 2, MANIFOLD_DEBUG/_TIMING); release is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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