fix: eliminate DiscreteFlyingEdgesClipper2D SMP garbage coords (+ per-thread vtkLabelMapLookup)#175
Merged
Merged
Conversation
…D SMP data race vtkDiscreteFlyingEdgesClipper2D shared ONE vtkLabelMapLookup instance (algo.LMap) across all worker threads. vtkLabelMapLookup::IsLabelValue writes the shared CachedValue/CachedOutValue/CachedOutValueInitialized members on every cache miss with no synchronization -- a data race. With cvista defaulting the vtkSMPTools backend to STDThread, this produced garbage/uninitialized point coordinates (the smp-parity validator measured max coordinate deviation ~3.4e37 on Linux vs ~1.2e10 on macOS: a platform-varying uninitialized read). Fix mirrors the existing solution in vtkSurfaceNets3D: give each SMP thread its own lookup. Pass1 now holds a vtkSMPThreadLocal<vtkLabelMapLookup<T>*>, constructs one instance per thread in Initialize() and frees them in Reduce(); ClassifyXEdges takes the per-thread lookup as a parameter. The algorithm now just records the contour label values (Values/NumContours) used to build those per-thread lookups. Only Pass1/ClassifyXEdges queried the lookup, so the other passes are untouched; the serial path is unchanged. Also harden the narrower sibling race in vtkSurfaceNets3D's OUTPUT_STYLE_SELECTED path (SelectWorker), which shared a single lMap inside a vtkSMPTools::For. It now uses a per-thread lazily-created lookup and frees them after the For. The default OUTPUT_STYLE_BOUNDARY path was already race-free. Validator: vtkDiscreteFlyingEdgesClipper2D is moved out of the known-issue list (markKnown removed) so the smp-parity gate now gates it. It keeps orderRelaxed=true because, like vtkDiscreteFlyingEdges2D/3D, its threaded emission order is still nondeterministic even though the geometry is now correct; the gate requires run-to-run stability plus an order-insensitive match to serial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jx5tEgvZZWWfDrGTuNmtDp
…root cause CI proved the per-thread vtkLabelMapLookup fix does NOT resolve the symptom: Clipper2D still emits garbage output point coordinates under STDThread at the identical magnitude (max coord dev ~1.2e10 on macOS, unchanged). That matches the original analysis — a label-cache flip changes point COUNT, not point VALUES — so the shared-LMap race, though genuinely fixed here, was never this bug's cause. Revert the validator classification: Clipper2D goes back to a documented, ungated known-issue (gate green again) with a corrected reason noting the LMap race is hardened but a separate threading defect (an uninitialized/racy write into the output point array) remains, root cause open. The per-thread LMap changes to vtkDiscreteFlyingEdgesClipper2D and vtkSurfaceNets3D (SELECTED mode) stay -- they remove real, ThreadSanitizer-flaggable data races on their own merits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jx5tEgvZZWWfDrGTuNmtDp
…bage coords The LMap per-thread fix was proven (by the smp-parity validator) NOT to change the garbage-output-coordinate symptom, so the real defect was elsewhere. It is a counted-but-unwritten output point slot, exposed only under the STDThread SMP backend. Pass1 (ClassifyXEdges) and Pass2 (ClassifyYEdges) COUNT output points per-dyad -- one per Inside dyad origin, per x/y edge split, and per interior point -- based purely on the dyad classification, independent of whether the surrounding pixel forms a non-empty (manifold) polygon case. The point WRITE (GenerateDyadPoints) and the VertUses-driven id assignment, however, only run inside Pass4's `if (numPolys > 0)` branch. When every pixel touching a counted dyad has an empty case (e.g. an isolated single-label pixel -> case 1 (1,0,0,0), which is numPolys==0), Pass1 reserves a point slot in the prefix sum that Pass4 never writes. Serial execution masks this: the freshly-allocated (WriteVoidPointer) buffer reads back as reproducible zeros, so serial-vs-serial is byte-identical and the phantom points look like harmless (0,0,0) orphans. Under the STDThread backend the point buffer lands on heap pages dirtied by worker scratch, so the unwritten slots read wild uninitialized floats (max coord dev ~3.4e37 on Linux / ~1.2e10 on macOS) that vary run-to-run -- exactly the observed symptom. This is the concrete divergence from the race-free sibling vtkDiscreteFlyingEdges2D: that filter only counts edge intersections that belong to non-empty cases, so it never reserves an unwritten slot. Fix: zero-initialize the NewPoints buffer immediately after allocation in ContourImage, so the unwritten phantom slots are deterministic (0) and identical to the serial result. The genuinely-used points are always written via GenerateDyadPoints into per-row disjoint output partitions, so this only touches the never-written padding slots and leaves correct geometry unchanged. Validator: vtkDiscreteFlyingEdgesClipper2D moves out of the known-issue list (markKnown removed) so the smp-parity gate now gates it. orderRelaxed stays true because, like vtkDiscreteFlyingEdges2D/3D, its threaded emission order is still nondeterministic even though the geometry is now correct; the gate requires run-to-run stability plus an order-insensitive match to serial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jx5tEgvZZWWfDrGTuNmtDp
akaszynski
force-pushed
the
fix/labelmaplookup-thread-race
branch
from
July 5, 2026 21:56
a8fabe3 to
1ddeb65
Compare
Member
Author
Self-review ✅Rebased onto Correctness — verified each change:
CI evidence: SMP parity green on linux + macOS — Merging on green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the SMP parity validator (#174). That validator found
vtkDiscreteFlyingEdgesClipper2Demits garbage output point coordinates under cvista's default STDThread backend (same point count as serial, wild values — max coord dev ~3.4e37 on Linux / ~1.2e10 on macOS, varying run-to-run). This PR fixes that, plus removes two related shared-cache data races found along the way.1. The real bug — counted-but-unwritten output points (the garbage coords)
Pass1/Pass2(ClassifyXEdges/ClassifyYEdges) count an output point per classified dyad (Inside origins, x/y edge splits, interior points) purely from dyad classification — independent of whether the surrounding pixel forms a non-empty polygon case. But the point write (GenerateDyadPoints) only runs insidePass4'sif (numPolys > 0)branch. So an isolated single-label pixel (case 1(1,0,0,0),numPolys==0) reserves a point slot in the prefix sum that Pass4 never writes.Serial execution masked it: the fresh
WriteVoidPointerbuffer reads back as reproducible zeros, so the phantom points looked like harmless(0,0,0)orphans and serial-vs-serial was byte-identical. Under STDThread the point buffer lands on heap pages dirtied by worker scratch, so those unwritten slots read wild uninitialized floats — the observed symptom. The distinguishing difference from the race-free siblingvtkDiscreteFlyingEdges2D: that filter only counts intersections belonging to non-empty cases, so it never reserves an unwritten slot.Fix: zero-initialize the
NewPointsbuffer right after allocation, so the never-written padding slots are deterministic(0,0,0)and identical to serial. Genuinely-used points are always written into per-row disjoint partitions, so real geometry is untouched. This eliminates the uninitialized-read UB and makes parallel output equal serial. (The phantom points still exist as unreferenced(0,0,0)orphans — a pre-existing logical over-count in the Flying-Edges counting, shared with stock VTK; this makes them deterministic rather than removing them. Removing the over-count is a deeper, riskier change left for later.)2. Two shared-cache data races (defensive, ThreadSanitizer-flaggable)
Both
vtkDiscreteFlyingEdgesClipper2DandvtkSurfaceNets3D'sOUTPUT_STYLE_SELECTEDpath shared onevtkLabelMapLookupacross all threads, whose cache (CachedValue/CachedOutValue) is written unsynchronized on every miss. Fixed by giving each SMP thread its own lookup viavtkSMPThreadLocal, mirroring how SurfaceNets3D's default BOUNDARY path already does it. (Note: the validator proved this race was not the cause of the garbage-coords symptom — #1 was — but it's a real race worth removing.)Result
The smp-parity gate (#174) now gates
vtkDiscreteFlyingEdgesClipper2D(moved out of known-issue) and it PASSES order-relaxed on Linux + macOS:gated-failures: 0. Its threaded geometry now equals serial and is run-to-run stable; only the emission order is thread-dependent, like the sibling DiscreteFlyingEdges filters.🤖 Generated with Claude Code