[geom] Fix TLS ownership and lifecycle - #1
Closed
agheata wants to merge 4 commits into
Closed
Conversation
Lazy pattern initialization can run while a different geometry manager is current. Register new matrices, including identity matrices, with the manager owning the divided volume so deleting an unrelated manager cannot invalidate the TLS cache. Preserve the existing CreateMatrix interface and cover the two-manager lifetime explicitly.
Keep the hot TLS slots for Pgon and Xtru as non-owning caches while moving their large buffers and polygons under shape ownership. ClearThreadData and shape destruction can now reclaim these allocations after navigation is quiescent. Cover cleanup and lazy rebuilding from stale TLS slots with multiple worker threads.
Document that monotonic TLS slot vectors retain their high-water size until the owning thread exits, while the shape-owned large allocations are reclaimed separately. Limit lazy-allocation claims to component scratch state and clarify that SetMaxThreads is still required for thread-safe manager navigation. Correct the cached pattern-matrix lifetime description as well.
Add Tristan Wenzel to the 6.42 contributor list. Sandro Wenzel is already present, so both contributors to the navigation optimization are credited without duplicating an existing entry.
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
1 participant
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.
This follows up the requested changes from the review of
root-project#22955 and is based directly on the current head of
twenzel/geom-false-sharing-fixes(3e31b2338b9). The large navigationscaling improvement from the original work is preserved; these commits
focus on ownership and lifecycle correctness around the new TLS design.
Changes
manager owning the divided volume instead of the ambient
gGeoManager.The existing
CreateMatrix()virtual interface is preserved.is current, deletes B, and then reuses A.
shape owns their large per-thread buffers and polygons. Cleanup and shape
destruction now reclaim those allocations.
high-water slots retained until thread exit, and that
SetMaxThreads()remains necessary for manager-level thread safety.
already listed there.
The large buffers are still allocated separately on cold first-touch, as
in the optimized branch. Only their ownership changes, so the steady-state
TLS lookup remains unchanged and lock-free. The mutex is used only for cold
allocation registration and cleanup.
Validation
cmake --build ... --target onepcm thread_navigation -j2ctest --output-on-failure -R '^gtest-geom-'suppressions and no unsuppressed diagnostics.
the reviewed PR head and about 16.5 kB with this follow-up; the remainder
is the documented small TLS high-water vector.
single-thread medians were 0.763 vs 0.760 million rays/s (-0.3%, within
dispersion), and 16-thread medians were 11.64 vs 11.85 million rays/s.
Checksums matched in every run.
git clang-format --diffreports no formatting changes for the completefollow-up range, using clang-format 18.1.3 and ROOT's
.clang-format.Development and review were assisted by Codex (GPT-5) under direct human
supervision.