Fix init_prism centroid; make test-prism's parallel loops thread-safe#81
Merged
Merged
Conversation
…aths Two related bugs surfaced by running the --enable-openmp test suite against test-prism, both pre-existing: 1) init_prism assigned prsm->centroid BEFORE the shift-to-o->center logic, then the shift only updated a local `centroid` variable. After construction, any prism whose center was modified ended up with prsm->centroid stale relative to the (shifted) vertices. 2) point_in_objectp, normal_to_object, and point_in_periodic_objectp each called geom_fix_object_ptr(&o) on every query. For prisms that path calls reinit_prism, which free()s + malloc()s shared per-prism arrays (vertices_p, vertices_top_p, ...). Under OpenMP the test was crashing with "malloc(): unaligned tcache chunk" / "free(): double free". This matches what PR NanoComp#75 already did for geom_get_bounding_box -- callers are now responsible for calling geom_fix_object_ptr after mutating o.center. test-prism: after the random-shift branch, call geom_fix_object_ptr on the block and prism once, single-threaded, before the parallel test loops run. Verified at OMP_NUM_THREADS = 1, 2, 4, 8, 16, 32, 64, 128, 166: all clean (0 point failures, 0 segment failures, no heap corruption). make check passes.
stevengj
reviewed
May 22, 2026
Per review feedback, point_in_objectp / normal_to_object / point_in_periodic_objectp are restored to their original behavior (call geom_fix_object_ptr internally) rather than being silently turned into thin aliases for the *_fixed_* variants. Their doc comments now state they are NOT thread-safe and direct concurrent callers at the *_fixed_* entry points. test-prism's parallel loops switched from point_in_objectp / normal_to_object to point_in_fixed_objectp / normal_to_fixed_object, which are pure reads and safe to call from multiple threads against an object that was fixed up once beforehand. Combined with the init_prism centroid fix in the previous commit, make check passes cleanly across OMP_NUM_THREADS = 1, 2, 4, 8, 16, 64, 166 (21 trials each: 0 point-inclusion failures, 0 segment failures, no heap-corruption signatures).
Luochenghuang
changed the title
Collaborator
|
The revised PR looks good to me. |
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
2 participants
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.
After #75 + #79,
test-prismwith--enable-openmpwas still failing atOMP_NUM_THREADS >= 2with heap corruption and intermittent point-inclusion mismatches. Two issues.1.
init_prismwritesprsm->centroidbefore the optional shiftThe line
prsm->centroid = centroid = ...happens before theelseblock that shifts bothverticesand the localcentroidto land ono->center. The shift only updates the local —prsm->centroidstays at the pre-shift value, while the vertex arrays reflect the shifted geometry. Block-vs-prism point-inclusion then disagrees on the difference between old and new placement.The pre-PR auto-fix in
point_in_objectpmasked this in serial code (the second call recomputedcentroidfrom the now-shifted vertices), but #79's parallel loop exposed it.Fix: move
prsm->centroid = centroid;to after the if/else. Idempotent in serial code too — not OpenMP-specific.2. test-prism's parallel loops were calling the thread-unsafe wrappers
point_in_objectp/normal_to_object/point_in_periodic_objectpcallgeom_fix_object_ptron every invocation, which for prisms doesfree/mallocon shared per-prism arrays. Two threads racing on the same prism corrupt the allocator — the original crash from #79.The wrappers are kept as-is (preserving API contract for callers that mutate object parameters between queries) but their doc comments now mark them as not thread-safe and direct concurrent callers at the
*_fixed_*variants.In test-prism:
test_point_inclusion: switch topoint_in_fixed_objectp.test_normal_to_object(disabled but still parallel): switch tonormal_to_fixed_object.run_unit_tests: callgeom_fix_object_ptronthe_blockandthe_prisminside theP_SHIFTblock, before the parallel loops run.Verification
make checkpasses. Three trials each atOMP_NUM_THREADS = 1, 2, 4, 8, 16, 64, 166: 21/21 clean (0/10000 points failed, 0/1000 segments failed, no heap-corruption signatures).