From 37cd8b01a4d442000fb79bf8d782f347bae3af02 Mon Sep 17 00:00:00 2001 From: Tristan Wenzel Date: Wed, 29 Jul 2026 12:35:49 +0200 Subject: [PATCH 1/6] [geom] Reduce thread false sharing and improve multithreaded TGeo navigation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit improves multithreaded TGeo navigation. The changes come from profiling a parallel geometry scan in ALICE: filling the material budget LUT on 28 cores now scales from 12× to 23× speedup (139 s -> 72 s). Two costs dominated. Every per-thread scratch lookup went through TGeoManager::ThreadId(), a non-inlined cross-library __tls_get_addr call paid on every boolean, section, and division query. In addition, the per-object ThreadData_t blocks were allocated back-to-back, so slots belonging to different threads often shared cache lines and invalidated each other on every write (false sharing), especially across sockets. Each object now gets a dense index, while the per-thread state lives in a single thread_local vector indexed by it. GetThreadData() becomes a header-inlined TLS read followed by an indexed load, and each thread owns its entire vector. Main changes: * TGeoBoolNode, TGeoVolumeAssembly, TGeoPatternFinder, TGeoPgon, TGeoXtru now use indexed thread-local storage. * TGeoPgon/TGeoXtru: add noexcept move constructors for ThreadData_t (which owns heap buffers, and for TGeoXtru also a TGeoPolygon aliasing them), so entries remain valid when the vector grows. * TGeoPatternFinder: reuse the transformation matrix across generations. The matrix is owned by the geometry manager and is never released, so creating a new one would leak one matrix per (thread, finder). Provisioning is no longer needed. ClearThreadData() now increments a generation counter, and each thread lazily rebuilds its slot on first access. As a result, CreateThreadData() and SetMaxThreads() are no longer required to support a given thread count: any number of threads now works out of the box. The generation counters are atomic because ClearThreadData() is const and may be called concurrently. Assisted-by: Claude Code (review, hardening and benchmarking) Supervised-by: Sandro Wenzel --- geom/geom/inc/TGeoBoolNode.h | 33 +++++---- geom/geom/inc/TGeoPatternFinder.h | 52 +++++++++----- geom/geom/inc/TGeoPgon.h | 48 ++++++++++--- geom/geom/inc/TGeoVolume.h | 39 ++++++----- geom/geom/inc/TGeoXtru.h | 57 +++++++++++---- geom/geom/src/TGeoBoolNode.cxx | 77 +------------------- geom/geom/src/TGeoPatternFinder.cxx | 65 +++++------------ geom/geom/src/TGeoPgon.cxx | 70 ++++++++----------- geom/geom/src/TGeoVolume.cxx | 86 +---------------------- geom/geom/src/TGeoXtru.cxx | 104 +++++++++++++++------------- 10 files changed, 262 insertions(+), 369 deletions(-) diff --git a/geom/geom/inc/TGeoBoolNode.h b/geom/geom/inc/TGeoBoolNode.h index bf3f5dbc005e0..c63b2aadb37b4 100644 --- a/geom/geom/inc/TGeoBoolNode.h +++ b/geom/geom/inc/TGeoBoolNode.h @@ -14,7 +14,8 @@ #include "TGeoShape.h" -#include +#include +#include #include // forward declarations @@ -23,6 +24,9 @@ class TGeoMatrix; class TGeoHMatrix; class TGeoBoolNode : public TObject { + static std::atomic fgInstanceCount; //! source of dense per-object indices + UInt_t fIndex{fgInstanceCount++}; //! dense index of this node into the per-thread vector + public: enum EGeoBoolType { kGeoUnion, @@ -30,14 +34,20 @@ class TGeoBoolNode : public TObject { kGeoSubtraction }; struct ThreadData_t { - Int_t fSelected; // ! selected branch - - ThreadData_t(); - ~ThreadData_t(); + Int_t fSelected{0}; //! selected branch }; - ThreadData_t &GetThreadData() const; - void ClearThreadData() const; - void CreateThreadData(Int_t nthreads); + + /// Per-thread scratch state, owned by the calling thread and indexed by this node. + /// Each thread owns its whole vector, so no two threads ever write the same cache line. + ThreadData_t &GetThreadData() const + { + thread_local std::vector tdata; + if (tdata.size() <= fIndex) + tdata.resize(std::max(fgInstanceCount.load(std::memory_order_relaxed), fIndex + 1)); + return tdata[fIndex]; + } + void ClearThreadData() const {} + void CreateThreadData(Int_t) {} private: TGeoBoolNode(const TGeoBoolNode &) = delete; @@ -51,11 +61,8 @@ class TGeoBoolNode : public TObject { mutable Int_t fNpoints{0}; /// fThreadData; /// +#include +#include #include #include "TGeoVolume.h" @@ -23,24 +24,43 @@ class TGeoMatrix; /// base finder class for patterns. A pattern is specifying a division type class TGeoPatternFinder : public TObject { + static std::atomic fgInstanceCount; //! source of dense per-object indices + UInt_t fIndex{fgInstanceCount++}; //! dense index of this finder into the per-thread vector + mutable std::atomic fGeneration{0}; //! bumped whenever the per-thread state must be rebuilt + public: struct ThreadData_t { - TGeoMatrix *fMatrix; /// tdata; + if (tdata.size() <= fIndex) + tdata.resize(std::max(fgInstanceCount.load(std::memory_order_relaxed), fIndex + 1)); + ThreadData_t &td = tdata[fIndex]; + if (td.fInitGen != fGeneration.load(std::memory_order_acquire)) + InitThreadSlot(td); + return td; + } + /// Invalidate the per-thread data. Each thread rebuilds its own slot lazily on next access, + /// so no cross-thread reach-in is needed. + void ClearThreadData() const { fGeneration.fetch_add(1, std::memory_order_release); } + /// No-op: per-thread data is allocated lazily, so no provisioning for a fixed thread count + /// is required and any number of threads works. + void CreateThreadData(Int_t) {} protected: + void InitThreadSlot(ThreadData_t &td) const; + enum EGeoPatternFlags { kPatternReflected = BIT(14), kPatternSpacedOut = BIT(15) }; Double_t fStep; // division step length Double_t fStart; // starting point on divided axis @@ -49,10 +69,6 @@ class TGeoPatternFinder : public TObject { Int_t fDivIndex; // index of first div. node TGeoVolume *fVolume; // volume to which applies - mutable std::vector fThreadData; /// +#include +#include #include class TGeoPgon : public TGeoPcon { + static std::atomic fgInstanceCount; //! source of dense per-object indices + UInt_t fIndex{fgInstanceCount++}; //! dense index of this shape into the per-thread vector + mutable std::atomic fGeneration{0}; //! bumped whenever the per-thread state must be rebuilt + public: struct ThreadData_t { - Int_t *fIntBuffer; /// tdata; + if (tdata.size() <= fIndex) + tdata.resize(std::max(fgInstanceCount.load(std::memory_order_relaxed), fIndex + 1)); + ThreadData_t &td = tdata[fIndex]; + if (td.fInitGen != fGeneration.load(std::memory_order_acquire)) + InitThreadSlot(td); + return td; + } + /// Invalidate the per-thread data. Each thread rebuilds its own slot lazily on next access. + void ClearThreadData() const override { fGeneration.fetch_add(1, std::memory_order_release); } + /// No-op: per-thread data is allocated lazily, so no provisioning for a fixed thread count + /// is required and any number of threads works. + void CreateThreadData(Int_t) override {} protected: + void InitThreadSlot(ThreadData_t &td) const; + // data members - Int_t fNedges; // number of edges (at least one) - mutable std::vector fThreadData; /// +#include #include #include @@ -315,23 +317,32 @@ class TGeoVolumeMulti : public TGeoVolume { //////////////////////////////////////////////////////////////////////////// class TGeoVolumeAssembly : public TGeoVolume { + static std::atomic fgInstanceCount; //! source of dense per-object indices + UInt_t fIndex{fgInstanceCount++}; //! dense index of this assembly into the per-thread vector + public: struct ThreadData_t { - Int_t fCurrent; /// tdata; + if (tdata.size() <= fIndex) + tdata.resize(std::max(fgInstanceCount.load(std::memory_order_relaxed), fIndex + 1)); + return tdata[fIndex]; + } + // ClearThreadData()/CreateThreadData() are deliberately not overridden: the assembly's own + // per-thread state is allocated lazily, and TGeoVolume's implementation still has to run so + // the shape and the division finder get their generation bumped. -protected: - mutable std::vector fThreadData; /// +#include +#include #include class TGeoPolygon; class TGeoXtru : public TGeoBBox { + static std::atomic fgInstanceCount; //! source of dense per-object indices + UInt_t fIndex{fgInstanceCount++}; //! dense index of this shape into the per-thread vector + mutable std::atomic fGeneration{0}; //! bumped whenever the per-thread state must be rebuilt + mutable std::atomic fIllegalChecked{kFALSE}; //! illegal-polygon warning already emitted + public: struct ThreadData_t { - Int_t fSeg; // !current segment [0,fNvert-1] - Int_t fIz; // !current z plane [0,fNz-1] - Double_t *fXc; // ![fNvert] current X positions for polygon vertices - Double_t *fYc; // ![fNvert] current Y positions for polygon vertices - TGeoPolygon *fPoly; // !polygon defining section shape + Int_t fSeg{0}; //! current segment [0,fNvert-1] + Int_t fIz{0}; //! current z plane [0,fNz-1] + Double_t *fXc{nullptr}; //![fNvert] current X positions for polygon vertices + Double_t *fYc{nullptr}; //![fNvert] current Y positions for polygon vertices + TGeoPolygon *fPoly{nullptr}; //! polygon defining section shape + Int_t fInitGen{-1}; //! generation this slot was last initialized for - ThreadData_t(); + ThreadData_t() = default; ~ThreadData_t(); + // Owns fXc/fYc/fPoly: movable so the slot can live in a resizable vector, not copyable. + ThreadData_t(ThreadData_t &&other) noexcept; + ThreadData_t &operator=(ThreadData_t &&other) noexcept; + ThreadData_t(const ThreadData_t &) = delete; + ThreadData_t &operator=(const ThreadData_t &) = delete; }; - ThreadData_t &GetThreadData() const; - void ClearThreadData() const override; - void CreateThreadData(Int_t nthreads) override; + + /// Per-thread scratch state, owned by the calling thread and indexed by this shape. + /// Hot path: a TLS read plus an indexed load; the cold rebuild lives in InitThreadSlot(). + ThreadData_t &GetThreadData() const + { + thread_local std::vector tdata; + if (tdata.size() <= fIndex) + tdata.resize(std::max(fgInstanceCount.load(std::memory_order_relaxed), fIndex + 1)); + ThreadData_t &td = tdata[fIndex]; + if (td.fInitGen != fGeneration.load(std::memory_order_acquire)) + InitThreadSlot(td); + return td; + } + /// Invalidate the per-thread data. Each thread rebuilds its own slot lazily on next access. + void ClearThreadData() const override + { + fGeneration.fetch_add(1, std::memory_order_release); + fIllegalChecked.store(kFALSE, std::memory_order_relaxed); + } + /// No-op: per-thread data is allocated lazily, so no provisioning for a fixed thread count + /// is required and any number of threads works. + void CreateThreadData(Int_t) override {} protected: + void InitThreadSlot(ThreadData_t &td) const; + // data members Int_t fNvert; // number of vertices of the 2D polygon (at least 3) Int_t fNz; // number of z planes (at least two) @@ -47,10 +80,6 @@ class TGeoXtru : public TGeoBBox { Double_t *fX0; //[fNz] array of X offsets (for each Z) Double_t *fY0; //[fNz] array of Y offsets (for each Z) - mutable std::vector fThreadData; /// guard(fMutex); - if (tid >= fThreadSize) { - Error("GetThreadData", "Thread id=%d bigger than maximum declared thread number %d. \nUse - TGeoManager::SetMaxThreads properly !!!", tid, fThreadSize); - } - if (tid >= fThreadSize) - { - fThreadData.resize(tid + 1); - fThreadSize = tid + 1; - } - if (fThreadData[tid] == 0) - { - if (fThreadData[tid] == 0) - fThreadData[tid] = new ThreadData_t; - } - */ - return *fThreadData[tid]; -} - -//////////////////////////////////////////////////////////////////////////////// - -void TGeoBoolNode::ClearThreadData() const -{ - std::lock_guard guard(fMutex); - std::vector::iterator i = fThreadData.begin(); - while (i != fThreadData.end()) { - delete *i; - ++i; - } - fThreadData.clear(); - fThreadSize = 0; -} - -//////////////////////////////////////////////////////////////////////////////// -/// Create thread data for n threads max. - -void TGeoBoolNode::CreateThreadData(Int_t nthreads) -{ - std::lock_guard guard(fMutex); - fThreadData.resize(nthreads); - fThreadSize = nthreads; - for (Int_t tid = 0; tid < nthreads; tid++) { - if (fThreadData[tid] == nullptr) { - fThreadData[tid] = new ThreadData_t; - } - } - // Propagate to components - if (fLeft) - fLeft->CreateThreadData(nthreads); - if (fRight) - fRight->CreateThreadData(nthreads); -} - +std::atomic TGeoBoolNode::fgInstanceCount{0}; //////////////////////////////////////////////////////////////////////////////// /// Set the selected branch. @@ -131,8 +63,6 @@ TGeoBoolNode::TGeoBoolNode() fRightMat = nullptr; fNpoints = 0; fPoints = nullptr; - fThreadSize = 0; - CreateThreadData(1); } //////////////////////////////////////////////////////////////////////////////// @@ -146,8 +76,6 @@ TGeoBoolNode::TGeoBoolNode(const char *expr1, const char *expr2) fRightMat = nullptr; fNpoints = 0; fPoints = nullptr; - fThreadSize = 0; - CreateThreadData(1); if (!MakeBranch(expr1, kTRUE)) { return; } @@ -166,8 +94,6 @@ TGeoBoolNode::TGeoBoolNode(TGeoShape *left, TGeoShape *right, TGeoMatrix *lmat, fLeftMat = lmat; fNpoints = 0; fPoints = nullptr; - fThreadSize = 0; - CreateThreadData(1); if (!fLeftMat) fLeftMat = gGeoIdentity; else @@ -195,7 +121,6 @@ TGeoBoolNode::~TGeoBoolNode() { if (fPoints) delete[] fPoints; - ClearThreadData(); } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/geom/src/TGeoPatternFinder.cxx b/geom/geom/src/TGeoPatternFinder.cxx index 9616aa2b7ad2d..5649fe5552ee0 100644 --- a/geom/geom/src/TGeoPatternFinder.cxx +++ b/geom/geom/src/TGeoPatternFinder.cxx @@ -36,56 +36,29 @@ on different axis. Implemented patterns are: #include "TGeoManager.h" #include "TMath.h" +std::atomic TGeoPatternFinder::fgInstanceCount{0}; //////////////////////////////////////////////////////////////////////////////// -/// Constructor. +/// (Re)build the per-thread scratch state for this finder into the given slot. +/// Cold path: runs once per (thread, finder, generation). -TGeoPatternFinder::ThreadData_t::ThreadData_t() : fMatrix(nullptr), fCurrent(-1), fNextIndex(-1) {} - -//////////////////////////////////////////////////////////////////////////////// -/// Destructor. - -TGeoPatternFinder::ThreadData_t::~ThreadData_t() -{ - // if (fMatrix != gGeoIdentity) delete fMatrix; -} - -//////////////////////////////////////////////////////////////////////////////// - -TGeoPatternFinder::ThreadData_t &TGeoPatternFinder::GetThreadData() const -{ - Int_t tid = TGeoManager::ThreadId(); - return *fThreadData[tid]; -} - -//////////////////////////////////////////////////////////////////////////////// - -void TGeoPatternFinder::ClearThreadData() const -{ - std::lock_guard guard(fMutex); - std::vector::iterator i = fThreadData.begin(); - while (i != fThreadData.end()) { - delete *i; - ++i; - } - fThreadData.clear(); - fThreadSize = 0; -} - -//////////////////////////////////////////////////////////////////////////////// -/// Create thread data for n threads max. - -void TGeoPatternFinder::CreateThreadData(Int_t nthreads) +void TGeoPatternFinder::InitThreadSlot(ThreadData_t &td) const { - std::lock_guard guard(fMutex); - fThreadData.resize(nthreads); - fThreadSize = nthreads; - for (Int_t tid = 0; tid < nthreads; tid++) { - if (fThreadData[tid] == nullptr) { - fThreadData[tid] = new ThreadData_t; - fThreadData[tid]->fMatrix = CreateMatrix(); - } + if (!td.fMatrix) { + // CreateMatrix() registers the new matrix with the geometry manager, which mutates a + // shared, unlocked TObjArray. Lazy initialization means several threads can reach this + // on first touch concurrently, so serialize the registration. Taken once per + // (thread, finder); steady-state navigation is lock-free. + static std::mutex sInitMutex; + std::lock_guard guard(sInitMutex); + td.fMatrix = CreateMatrix(); } + // A generation bump only invalidates the cached division indices. The matrix stays valid and + // is deliberately reused: it is owned by the geometry manager and never released, so creating + // a fresh one here would leak one matrix per (thread, finder) on every ClearThreadData(). + td.fCurrent = -1; + td.fNextIndex = -1; + td.fInitGen = fGeneration.load(std::memory_order_acquire); } //////////////////////////////////////////////////////////////////////////////// @@ -99,7 +72,6 @@ TGeoPatternFinder::TGeoPatternFinder() fStart = 0; fEnd = 0; fVolume = nullptr; - fThreadSize = 0; } //////////////////////////////////////////////////////////////////////////////// @@ -113,7 +85,6 @@ TGeoPatternFinder::TGeoPatternFinder(TGeoVolume *vol, Int_t ndiv) fStep = 0; fStart = 0; fEnd = 0; - fThreadSize = 0; } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/geom/src/TGeoPgon.cxx b/geom/geom/src/TGeoPgon.cxx index 5444113c2ed40..05130b9b9d14e 100644 --- a/geom/geom/src/TGeoPgon.cxx +++ b/geom/geom/src/TGeoPgon.cxx @@ -66,11 +66,7 @@ polygons, between `phi1` and `phi1+dphi.` #include "TBuffer3DTypes.h" #include "TMath.h" - -//////////////////////////////////////////////////////////////////////////////// -/// Constructor. - -TGeoPgon::ThreadData_t::ThreadData_t() : fIntBuffer(nullptr), fDblBuffer(nullptr) {} +std::atomic TGeoPgon::fgInstanceCount{0}; //////////////////////////////////////////////////////////////////////////////// /// Destructor. @@ -82,44 +78,45 @@ TGeoPgon::ThreadData_t::~ThreadData_t() } //////////////////////////////////////////////////////////////////////////////// +/// Move constructor. Steals the owned buffers; the moved-from slot is left empty +/// and uninitialized (fInitGen = -1). -TGeoPgon::ThreadData_t &TGeoPgon::GetThreadData() const +TGeoPgon::ThreadData_t::ThreadData_t(ThreadData_t &&other) noexcept + : fIntBuffer(other.fIntBuffer), fDblBuffer(other.fDblBuffer), fInitGen(other.fInitGen) { - Int_t tid = TGeoManager::ThreadId(); - return *fThreadData[tid]; + other.fIntBuffer = nullptr; + other.fDblBuffer = nullptr; + other.fInitGen = -1; } //////////////////////////////////////////////////////////////////////////////// +/// Move assignment. Releases the current buffers before stealing the source's. -void TGeoPgon::ClearThreadData() const +TGeoPgon::ThreadData_t &TGeoPgon::ThreadData_t::operator=(ThreadData_t &&other) noexcept { - std::lock_guard guard(fMutex); - std::vector::iterator i = fThreadData.begin(); - while (i != fThreadData.end()) { - delete *i; - ++i; - } - fThreadData.clear(); - fThreadSize = 0; + if (this != &other) { + delete[] fIntBuffer; + delete[] fDblBuffer; + fIntBuffer = other.fIntBuffer; + fDblBuffer = other.fDblBuffer; + fInitGen = other.fInitGen; + other.fIntBuffer = nullptr; + other.fDblBuffer = nullptr; + other.fInitGen = -1; + } + return *this; } //////////////////////////////////////////////////////////////////////////////// -/// Create thread data for n threads max. +/// (Re)build the per-thread scratch buffers for this shape into the given slot. +/// Cold path: runs once per (thread, shape, generation). -void TGeoPgon::CreateThreadData(Int_t nthreads) +void TGeoPgon::InitThreadSlot(ThreadData_t &td) const { - if (fThreadSize) - ClearThreadData(); - std::lock_guard guard(fMutex); - fThreadData.resize(nthreads); - fThreadSize = nthreads; - for (Int_t tid = 0; tid < nthreads; tid++) { - if (fThreadData[tid] == nullptr) { - fThreadData[tid] = new ThreadData_t; - fThreadData[tid]->fIntBuffer = new Int_t[fNedges + 10]; - fThreadData[tid]->fDblBuffer = new Double_t[fNedges + 10]; - } - } + td = ThreadData_t{}; // release any buffers left from a previous generation + td.fIntBuffer = new Int_t[fNedges + 10]; + td.fDblBuffer = new Double_t[fNedges + 10]; + td.fInitGen = fGeneration.load(std::memory_order_acquire); } //////////////////////////////////////////////////////////////////////////////// @@ -129,7 +126,6 @@ TGeoPgon::TGeoPgon() { SetShapeBit(TGeoShape::kGeoPgon); fNedges = 0; - fThreadSize = 0; } //////////////////////////////////////////////////////////////////////////////// @@ -139,8 +135,6 @@ TGeoPgon::TGeoPgon(Double_t phi, Double_t dphi, Int_t nedges, Int_t nz) : TGeoPc { SetShapeBit(TGeoShape::kGeoPgon); fNedges = nedges; - fThreadSize = 0; - CreateThreadData(1); } //////////////////////////////////////////////////////////////////////////////// @@ -151,8 +145,6 @@ TGeoPgon::TGeoPgon(const char *name, Double_t phi, Double_t dphi, Int_t nedges, { SetShapeBit(TGeoShape::kGeoPgon); fNedges = nedges; - fThreadSize = 0; - CreateThreadData(1); } //////////////////////////////////////////////////////////////////////////////// @@ -171,8 +163,6 @@ TGeoPgon::TGeoPgon(Double_t *param) : TGeoPcon("") SetShapeBit(TGeoShape::kGeoPgon); SetDimensions(param); ComputeBBox(); - fThreadSize = 0; - CreateThreadData(1); } //////////////////////////////////////////////////////////////////////////////// @@ -466,8 +456,6 @@ TGeoPgon::DistFromInside(const Double_t *point, const Double_t *dir, Int_t iact, ipl++; } Double_t stepmax = step; - if (!fThreadSize) - ((TGeoPgon *)this)->CreateThreadData(1); ThreadData_t &td = GetThreadData(); Double_t *sph = td.fDblBuffer; Int_t *iph = td.fIntBuffer; @@ -1253,8 +1241,6 @@ TGeoPgon::DistFromOutside(const Double_t *point, const Double_t *dir, Int_t iact } } } - if (!fThreadSize) - ((TGeoPgon *)this)->CreateThreadData(1); ThreadData_t &td = GetThreadData(); Double_t *sph = td.fDblBuffer; Int_t *iph = td.fIntBuffer; diff --git a/geom/geom/src/TGeoVolume.cxx b/geom/geom/src/TGeoVolume.cxx index d640852acaa50..34ca4fbaaf93d 100644 --- a/geom/geom/src/TGeoVolume.cxx +++ b/geom/geom/src/TGeoVolume.cxx @@ -2973,91 +2973,11 @@ void TGeoVolumeMulti::SetVisibility(Bool_t vis) } } -//////////////////////////////////////////////////////////////////////////////// -/// Constructor. - -TGeoVolumeAssembly::ThreadData_t::ThreadData_t() : fCurrent(-1), fNext(-1) {} - -//////////////////////////////////////////////////////////////////////////////// -/// Destructor. - -TGeoVolumeAssembly::ThreadData_t::~ThreadData_t() {} - -//////////////////////////////////////////////////////////////////////////////// - -TGeoVolumeAssembly::ThreadData_t &TGeoVolumeAssembly::GetThreadData() const -{ - Int_t tid = TGeoManager::ThreadId(); - return *fThreadData[tid]; -} - -//////////////////////////////////////////////////////////////////////////////// - -void TGeoVolumeAssembly::ClearThreadData() const -{ - std::lock_guard guard(fMutex); - TGeoVolume::ClearThreadData(); - std::vector::iterator i = fThreadData.begin(); - while (i != fThreadData.end()) { - delete *i; - ++i; - } - fThreadData.clear(); - fThreadSize = 0; -} - -//////////////////////////////////////////////////////////////////////////////// - -void TGeoVolumeAssembly::CreateThreadData(Int_t nthreads) -{ - std::lock_guard guard(fMutex); - // Create assembly thread data here - fThreadData.resize(nthreads); - fThreadSize = nthreads; - for (Int_t tid = 0; tid < nthreads; tid++) { - if (fThreadData[tid] == nullptr) { - fThreadData[tid] = new ThreadData_t; - } - } - TGeoVolume::CreateThreadData(nthreads); -} - -//////////////////////////////////////////////////////////////////////////////// - -Int_t TGeoVolumeAssembly::GetCurrentNodeIndex() const -{ - return fThreadData[TGeoManager::ThreadId()]->fCurrent; -} - -//////////////////////////////////////////////////////////////////////////////// - -Int_t TGeoVolumeAssembly::GetNextNodeIndex() const -{ - return fThreadData[TGeoManager::ThreadId()]->fNext; -} - -//////////////////////////////////////////////////////////////////////////////// - -void TGeoVolumeAssembly::SetCurrentNodeIndex(Int_t index) -{ - fThreadData[TGeoManager::ThreadId()]->fCurrent = index; -} - -//////////////////////////////////////////////////////////////////////////////// - -void TGeoVolumeAssembly::SetNextNodeIndex(Int_t index) -{ - fThreadData[TGeoManager::ThreadId()]->fNext = index; -} - +std::atomic TGeoVolumeAssembly::fgInstanceCount{0}; //////////////////////////////////////////////////////////////////////////////// /// Default constructor -TGeoVolumeAssembly::TGeoVolumeAssembly() : TGeoVolume() -{ - fThreadSize = 0; - CreateThreadData(1); -} +TGeoVolumeAssembly::TGeoVolumeAssembly() : TGeoVolume() {} //////////////////////////////////////////////////////////////////////////////// /// Constructor. Just the name has to be provided. Assemblies does not have their own @@ -3070,8 +2990,6 @@ TGeoVolumeAssembly::TGeoVolumeAssembly(const char *name) : TGeoVolume() fShape = new TGeoShapeAssembly(this); if (fGeoManager) fNumber = fGeoManager->AddVolume(this); - fThreadSize = 0; - CreateThreadData(1); } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/geom/src/TGeoXtru.cxx b/geom/geom/src/TGeoXtru.cxx index 36f16eb751fdb..e829a285d881c 100644 --- a/geom/geom/src/TGeoXtru.cxx +++ b/geom/geom/src/TGeoXtru.cxx @@ -101,11 +101,9 @@ Double_t y0, Double_t scale); #include "TGeoManager.h" #include "TGeoVolume.h" #include "TGeoPolygon.h" +#include "TROOT.h" -//////////////////////////////////////////////////////////////////////////////// -/// Constructor. - -TGeoXtru::ThreadData_t::ThreadData_t() : fSeg(0), fIz(0), fXc(nullptr), fYc(nullptr), fPoly(nullptr) {} +std::atomic TGeoXtru::fgInstanceCount{0}; //////////////////////////////////////////////////////////////////////////////// /// Destructor. @@ -114,57 +112,73 @@ TGeoXtru::ThreadData_t::~ThreadData_t() { delete[] fXc; delete[] fYc; - delete fPoly; + // fPoly is a TObject, so deleting it walks ROOT's cleanup machinery (RecursiveRemove, + // TObjArray::Delete). This slot lives in a thread_local vector and can therefore be + // destroyed at thread/process exit, possibly after ROOT's globals have been torn down, + // where that machinery reads freed state. Only delete while ROOT is still alive -- at + // shutdown the OS reclaims this small per-thread buffer anyway. The in-run rebuild path + // (move-assignment in InitThreadSlot) always runs while ROOT is up, so this guard only + // ever skips the very last teardown, never steady-state churn. + // Same "is ROOT still there" test that TDirectory and TGenericClassInfo use. + if (fPoly && ROOT::Internal::gROOTLocal) + delete fPoly; } //////////////////////////////////////////////////////////////////////////////// +/// Move constructor. Steals the owned resources; the moved-from slot is left empty and +/// uninitialized (fInitGen = -1). fPoly keeps pointing at fXc/fYc, which are not relocated. -TGeoXtru::ThreadData_t &TGeoXtru::GetThreadData() const +TGeoXtru::ThreadData_t::ThreadData_t(ThreadData_t &&other) noexcept + : fSeg(other.fSeg), fIz(other.fIz), fXc(other.fXc), fYc(other.fYc), fPoly(other.fPoly), fInitGen(other.fInitGen) { - if (!fThreadSize) - ((TGeoXtru *)this)->CreateThreadData(1); - Int_t tid = TGeoManager::ThreadId(); - return *fThreadData[tid]; + other.fXc = nullptr; + other.fYc = nullptr; + other.fPoly = nullptr; + other.fInitGen = -1; } //////////////////////////////////////////////////////////////////////////////// +/// Move assignment. Releases the current resources before stealing the source's. -void TGeoXtru::ClearThreadData() const +TGeoXtru::ThreadData_t &TGeoXtru::ThreadData_t::operator=(ThreadData_t &&other) noexcept { - std::lock_guard guard(fMutex); - std::vector::iterator i = fThreadData.begin(); - while (i != fThreadData.end()) { - delete *i; - ++i; + if (this != &other) { + delete[] fXc; + delete[] fYc; + delete fPoly; + fSeg = other.fSeg; + fIz = other.fIz; + fXc = other.fXc; + fYc = other.fYc; + fPoly = other.fPoly; + fInitGen = other.fInitGen; + other.fXc = nullptr; + other.fYc = nullptr; + other.fPoly = nullptr; + other.fInitGen = -1; } - fThreadData.clear(); - fThreadSize = 0; + return *this; } //////////////////////////////////////////////////////////////////////////////// -/// Create thread data for n threads max. +/// (Re)build the per-thread scratch state for this shape into the given slot. +/// Cold path: runs once per (thread, shape, generation). -void TGeoXtru::CreateThreadData(Int_t nthreads) +void TGeoXtru::InitThreadSlot(ThreadData_t &td) const { - std::lock_guard guard(fMutex); - fThreadData.resize(nthreads); - fThreadSize = nthreads; - for (Int_t tid = 0; tid < nthreads; tid++) { - if (fThreadData[tid] == nullptr) { - fThreadData[tid] = new ThreadData_t; - ThreadData_t &td = *fThreadData[tid]; - td.fXc = new Double_t[fNvert]; - td.fYc = new Double_t[fNvert]; - memcpy(td.fXc, fX, fNvert * sizeof(Double_t)); - memcpy(td.fYc, fY, fNvert * sizeof(Double_t)); - td.fPoly = new TGeoPolygon(fNvert); - td.fPoly->SetXY(td.fXc, td.fYc); // initialize with current coordinates - td.fPoly->FinishPolygon(); - if (tid == 0 && td.fPoly->IsIllegalCheck()) { - Error("DefinePolygon", "Shape %s of type XTRU has an illegal polygon.", GetName()); - } - } - } + td = ThreadData_t{}; // release anything left from a previous generation + td.fXc = new Double_t[fNvert]; + td.fYc = new Double_t[fNvert]; + memcpy(td.fXc, fX, fNvert * sizeof(Double_t)); + memcpy(td.fYc, fY, fNvert * sizeof(Double_t)); + td.fPoly = new TGeoPolygon(fNvert); + td.fPoly->SetXY(td.fXc, td.fYc); // initialize with current coordinates + td.fPoly->FinishPolygon(); + // The polygon is identical in every thread, so report an illegal one exactly once + // instead of once per thread (previously: only for thread id 0). + if (td.fPoly->IsIllegalCheck() && !fIllegalChecked.exchange(kTRUE, std::memory_order_relaxed)) + Error("DefinePolygon", "Shape %s of type XTRU has an illegal polygon.", GetName()); + td.fInitGen = fGeneration.load(std::memory_order_acquire); } //////////////////////////////////////////////////////////////////////////////// @@ -195,9 +209,7 @@ TGeoXtru::TGeoXtru() fZ(nullptr), fScale(nullptr), fX0(nullptr), - fY0(nullptr), - fThreadData(0), - fThreadSize(0) + fY0(nullptr) { SetShapeBit(TGeoShape::kGeoXtru); } @@ -215,9 +227,7 @@ TGeoXtru::TGeoXtru(Int_t nz) fZ(new Double_t[nz]), fScale(new Double_t[nz]), fX0(new Double_t[nz]), - fY0(new Double_t[nz]), - fThreadData(0), - fThreadSize(0) + fY0(new Double_t[nz]) { SetShapeBit(TGeoShape::kGeoXtru); if (nz < 2) { @@ -251,9 +261,7 @@ TGeoXtru::TGeoXtru(Double_t *param) fZ(nullptr), fScale(nullptr), fX0(nullptr), - fY0(nullptr), - fThreadData(0), - fThreadSize(0) + fY0(nullptr) { SetShapeBit(TGeoShape::kGeoXtru); SetDimensions(param); From 3e31b2338b9bfcd80526b406eff9bf01efaed95a Mon Sep 17 00:00:00 2001 From: Tristan Wenzel Date: Wed, 29 Jul 2026 12:35:49 +0200 Subject: [PATCH 2/6] [geom] Add a multi-threaded navigation regression test This commit provides a test exercising multithreaded TGeo navigation. Eight threads navigating the same geometry must give exactly the single-threaded answer. Covers TGeoXtru, TGeoPgon, TGeoVolumeAssembly, TGeoBoolNode (composite shape) and TGeoPatternFinder (divided volume). The threads book their navigators lazily, so it also exercises AddNavigator() against the navigator-map readers. Assisted-by: Claude Code (review, hardening and benchmarking) Supervised-by: Sandro Wenzel --- geom/test/CMakeLists.txt | 4 + geom/test/test_thread_navigation.cxx | 188 +++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 geom/test/test_thread_navigation.cxx diff --git a/geom/test/CMakeLists.txt b/geom/test/CMakeLists.txt index c52ed9610d743..11e4c69f8a083 100644 --- a/geom/test/CMakeLists.txt +++ b/geom/test/CMakeLists.txt @@ -25,6 +25,10 @@ ROOT_ADD_GTEST(overlap_navigation test_overlap_navigation.cxx LIBRARIES Geom) +ROOT_ADD_GTEST(thread_navigation + test_thread_navigation.cxx + LIBRARIES Geom) + if(imt) ROOT_ADD_GTEST(manager_lifetime test_manager_lifetime.cxx diff --git a/geom/test/test_thread_navigation.cxx b/geom/test/test_thread_navigation.cxx new file mode 100644 index 0000000000000..d01691526de78 --- /dev/null +++ b/geom/test/test_thread_navigation.cxx @@ -0,0 +1,188 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +/** + Navigating the same geometry from several threads must give exactly the same answer as + navigating it from one. This exercises every class that keeps per-thread scratch state: + TGeoXtru, TGeoPgon, TGeoVolumeAssembly, TGeoBoolNode (via a composite shape) and + TGeoPatternFinder (via a divided volume). + + Worth running under ThreadSanitizer: the threads book their navigators lazily, so this + also covers concurrent TGeoManager::AddNavigator() against navigator-map readers. +*/ + +namespace { + +/// Geometry containing one instance of each shape family that owns per-thread data. +TGeoManager *MakeGeometry() +{ + auto *geom = new TGeoManager("mt_nav_geom", "geometry for MT navigation test"); + + auto *matVac = new TGeoMaterial("Vacuum", 0, 0, 0); + auto *matAl = new TGeoMaterial("Al", 26.98, 13, 2.7); + auto *vac = new TGeoMedium("Vacuum", 1, matVac); + auto *alu = new TGeoMedium("Aluminium", 2, matAl); + + TGeoVolume *top = geom->MakeBox("TOP", vac, 100., 100., 100.); + geom->SetTopVolume(top); + + // --- TGeoXtru: convex, simple polygon extruded over two sections + Double_t xv[5] = {-10., -5., 5., 10., 0.}; + Double_t yv[5] = {-6., -10., -10., -6., 10.}; + auto *xtru = new TGeoXtru(2); + xtru->DefinePolygon(5, xv, yv); + xtru->DefineSection(0, -20., 0., 0., 1.); + xtru->DefineSection(1, 20., 0., 0., 1.); + top->AddNode(new TGeoVolume("XTRU", xtru, alu), 1, new TGeoTranslation(-45., 0., 0.)); + + // --- TGeoPgon + auto *pgon = new TGeoPgon("pgon", 0., 360., 8, 2); + pgon->DefineSection(0, -20., 5., 12.); + pgon->DefineSection(1, 20., 5., 12.); + top->AddNode(new TGeoVolume("PGON", pgon, alu), 1, new TGeoTranslation(45., 0., 0.)); + + // --- TGeoBoolNode, through a composite shape + new TGeoBBox("cbox", 12., 12., 12.); + new TGeoTube("ctub", 0., 6., 20.); + auto *comp = new TGeoCompositeShape("comp", "cbox - ctub"); + top->AddNode(new TGeoVolume("COMP", comp, alu), 1, new TGeoTranslation(0., 45., 0.)); + + // --- TGeoPatternFinder, through a divided volume + TGeoVolume *slab = geom->MakeBox("SLAB", alu, 20., 5., 5.); + slab->Divide("SLABDIV", 1, 10, -20., 4.); + top->AddNode(slab, 1, new TGeoTranslation(0., -45., 0.)); + + // --- TGeoVolumeAssembly + auto *assembly = new TGeoVolumeAssembly("ASSEMBLY"); + TGeoVolume *brick = geom->MakeBox("BRICK", alu, 3., 3., 3.); + for (int i = 0; i < 6; ++i) + assembly->AddNode(brick, i + 1, new TGeoTranslation(0., 0., -25. + 10. * i)); + top->AddNode(assembly, 1, new TGeoTranslation(0., 0., 0.)); + + geom->CloseGeometry(); + return geom; +} + +struct Ray { + Double_t point[3]; + Double_t dir[3]; +}; + +/// Deterministic fan of rays aimed at each shape, so every class with per-thread state is +/// actually traversed. Start points sit 40 cm from the target, well inside the world volume. +std::vector MakeRays() +{ + const Double_t targets[][3] = { + {-45., 0., 0.}, {-45., 0., 10.}, {-45., 3., -10.}, // XTRU + {45., 0., 0.}, {45., 0., 10.}, {45., 8., -10.}, // PGON + {0., 45., 0.}, {0., 45., 8.}, {8., 45., 0.}, // composite (box minus tube) + {0., -45., 0.}, {5., -45., 0.}, {-5., -45., 2.}, // divided slab + {0., 0., -25.}, {0., 0., -5.}, {0., 0., 15.}, // assembly bricks + }; + std::vector rays; + for (const auto &t : targets) { + for (int i = 0; i < 12; ++i) { + const double phi = 2. * M_PI * i / 12.; + const double theta = 0.4 + 0.15 * (i % 5); + const double d[3] = {std::sin(theta) * std::cos(phi), std::sin(theta) * std::sin(phi), std::cos(theta)}; + Ray ray; + for (int k = 0; k < 3; ++k) { + ray.point[k] = t[k] + 40. * d[k]; + ray.dir[k] = -d[k]; + } + rays.push_back(ray); + } + } + return rays; +} + +struct RayResult { + Int_t nsteps{0}; + Double_t pathlen{0.}; + Long64_t checksum{0}; // sequence of volumes traversed + + bool operator==(const RayResult &o) const + { + return nsteps == o.nsteps && checksum == o.checksum && std::abs(pathlen - o.pathlen) < 1e-9; + } +}; + +RayResult ShootRay(TGeoNavigator *nav, const Ray &ray) +{ + RayResult res; + nav->InitTrack(ray.point, ray.dir); + while (!nav->IsOutside() && res.nsteps < 500) { + TGeoNode *node = nav->GetCurrentNode(); + res.checksum = res.checksum * 31 + (node ? node->GetVolume()->GetNumber() : -1); + nav->FindNextBoundaryAndStep(1.e6, kFALSE); + res.pathlen += nav->GetStep(); + ++res.nsteps; + if (!nav->IsOnBoundary()) + break; + } + return res; +} + +} // namespace + +TEST(Geometry, MultiThreadedNavigationMatchesSerial) +{ + TGeoManager *geom = MakeGeometry(); + ASSERT_NE(geom, nullptr); + const std::vector rays = MakeRays(); + + // Reference: single-threaded, default navigator. + std::vector reference; + reference.reserve(rays.size()); + for (const Ray &ray : rays) + reference.push_back(ShootRay(geom->GetCurrentNavigator(), ray)); + + // Rays that miss every object legitimately cross a single boundary (the world). Guard against + // a vacuous comparison by requiring that a good share of them actually traverse the shapes. + const size_t nTraversing = + std::count_if(reference.begin(), reference.end(), [](const RayResult &r) { return r.nsteps > 2; }); + ASSERT_GT(nTraversing, reference.size() / 2); + + constexpr int kNThreads = 8; + geom->SetMaxThreads(kNThreads); + + std::vector> perThread(kNThreads); + std::vector threads; + threads.reserve(kNThreads); + for (int t = 0; t < kNThreads; ++t) { + threads.emplace_back([&, t] { + // Booked lazily and concurrently, exactly as a task-parallel workload would. + TGeoNavigator *nav = geom->AddNavigator(); + perThread[t].reserve(rays.size()); + for (const Ray &ray : rays) + perThread[t].push_back(ShootRay(nav, ray)); + }); + } + for (std::thread &th : threads) + th.join(); + + for (int t = 0; t < kNThreads; ++t) { + ASSERT_EQ(perThread[t].size(), reference.size()) << "thread " << t; + for (size_t i = 0; i < reference.size(); ++i) + EXPECT_TRUE(perThread[t][i] == reference[i]) << "thread " << t << ", ray " << i; + } + + delete geom; +} From c290c11e44aca14206f9354f3adbfe55133c5cea Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 14:45:21 +0200 Subject: [PATCH 3/6] [geom] Bind pattern matrices to owner 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. --- geom/geom/inc/TGeoPatternFinder.h | 8 ++- geom/geom/src/TGeoPatternFinder.cxx | 77 ++++++++++++++++++++-------- geom/test/test_thread_navigation.cxx | 66 ++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 23 deletions(-) diff --git a/geom/geom/inc/TGeoPatternFinder.h b/geom/geom/inc/TGeoPatternFinder.h index f9c508311d86b..b1dbb0481a4d6 100644 --- a/geom/geom/inc/TGeoPatternFinder.h +++ b/geom/geom/inc/TGeoPatternFinder.h @@ -60,8 +60,14 @@ class TGeoPatternFinder : public TObject { protected: void InitThreadSlot(ThreadData_t &td) const; + TGeoManager *GetOwnerManager() const { return fVolume ? fVolume->GetGeoManager() : nullptr; } + TGeoMatrix *GetOwnerIdentity() const; + void RegisterMatrix(TGeoMatrix *matrix) const; - enum EGeoPatternFlags { kPatternReflected = BIT(14), kPatternSpacedOut = BIT(15) }; + enum EGeoPatternFlags { + kPatternReflected = BIT(14), + kPatternSpacedOut = BIT(15) + }; Double_t fStep; // division step length Double_t fStart; // starting point on divided axis Double_t fEnd; // ending point diff --git a/geom/geom/src/TGeoPatternFinder.cxx b/geom/geom/src/TGeoPatternFinder.cxx index 5649fe5552ee0..3b58bc7fe8408 100644 --- a/geom/geom/src/TGeoPatternFinder.cxx +++ b/geom/geom/src/TGeoPatternFinder.cxx @@ -44,6 +44,11 @@ std::atomic TGeoPatternFinder::fgInstanceCount{0}; void TGeoPatternFinder::InitThreadSlot(ThreadData_t &td) const { + TGeoManager *manager = GetOwnerManager(); + if (!manager) { + Error("InitThreadSlot", "Pattern finder has no owning geometry manager"); + return; + } if (!td.fMatrix) { // CreateMatrix() registers the new matrix with the geometry manager, which mutates a // shared, unlocked TObjArray. Lazy initialization means several threads can reach this @@ -61,6 +66,34 @@ void TGeoPatternFinder::InitThreadSlot(ThreadData_t &td) const td.fInitGen = fGeneration.load(std::memory_order_acquire); } +//////////////////////////////////////////////////////////////////////////////// +/// Return the identity matrix owned by this finder's geometry manager. + +TGeoMatrix *TGeoPatternFinder::GetOwnerIdentity() const +{ + TGeoManager *manager = GetOwnerManager(); + return manager ? static_cast(manager->GetListOfMatrices()->At(0)) : nullptr; +} + +//////////////////////////////////////////////////////////////////////////////// +/// Register a lazily-created pattern matrix with the manager owning this finder's volume. + +void TGeoPatternFinder::RegisterMatrix(TGeoMatrix *matrix) const +{ + TGeoManager *manager = GetOwnerManager(); + if (!manager || !matrix) + return; + if (!matrix->IsRegistered()) { + manager->RegisterMatrix(matrix); + matrix->SetBit(TGeoMatrix::kGeoRegistered); + } + if (matrix->IsCombi()) { + TGeoRotation *rotation = static_cast(matrix)->GetRotation(); + if (rotation && rotation->IsRotation()) + RegisterMatrix(rotation); + } +} + //////////////////////////////////////////////////////////////////////////////// /// Default constructor @@ -271,11 +304,11 @@ TGeoMatrix *TGeoPatternX::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -465,11 +498,11 @@ TGeoMatrix *TGeoPatternY::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -655,11 +688,11 @@ TGeoMatrix *TGeoPatternZ::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -918,11 +951,11 @@ TGeoMatrix *TGeoPatternParaX::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1096,11 +1129,11 @@ TGeoMatrix *TGeoPatternParaY::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1278,11 +1311,11 @@ TGeoMatrix *TGeoPatternParaZ::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1467,11 +1500,11 @@ TGeoMatrix *TGeoPatternTrapZ::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + RegisterMatrix(combi); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1631,7 +1664,7 @@ void TGeoPatternCylR::SavePrimitive(std::ostream &out, Option_t * /*option*/ /*= TGeoMatrix *TGeoPatternCylR::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// @@ -1827,11 +1860,11 @@ TGeoMatrix *TGeoPatternCylPhi::CreateMatrix() const { if (!IsReflected()) { TGeoRotation *matrix = new TGeoRotation(); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoRotation *rot = new TGeoRotation(); - rot->RegisterYourself(); + RegisterMatrix(rot); rot->ReflectZ(kTRUE); rot->ReflectZ(kFALSE); return rot; @@ -1949,7 +1982,7 @@ void TGeoPatternSphR::SavePrimitive(std::ostream &out, Option_t * /*option*/ /*= TGeoMatrix *TGeoPatternSphR::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// @@ -2062,7 +2095,7 @@ void TGeoPatternSphTheta::SavePrimitive(std::ostream &out, Option_t * /*option*/ TGeoMatrix *TGeoPatternSphTheta::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// @@ -2241,11 +2274,11 @@ TGeoMatrix *TGeoPatternSphPhi::CreateMatrix() const { if (!IsReflected()) { TGeoRotation *matrix = new TGeoRotation(); - matrix->RegisterYourself(); + RegisterMatrix(matrix); return matrix; } TGeoRotation *rot = new TGeoRotation(); - rot->RegisterYourself(); + RegisterMatrix(rot); rot->ReflectZ(kTRUE); rot->ReflectZ(kFALSE); return rot; @@ -2342,7 +2375,7 @@ TGeoNode *TGeoPatternHoneycomb::FindNode(Double_t * /*point*/, const Double_t * TGeoMatrix *TGeoPatternHoneycomb::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/test/test_thread_navigation.cxx b/geom/test/test_thread_navigation.cxx index d01691526de78..4f02d5797ddba 100644 --- a/geom/test/test_thread_navigation.cxx +++ b/geom/test/test_thread_navigation.cxx @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -80,6 +81,39 @@ TGeoManager *MakeGeometry() return geom; } +struct GeometryWithFinders { + TGeoManager *manager; + TGeoPatternFinder *linearFinder; + TGeoPatternFinder *radialFinder; +}; + +/// Build two divided volumes without touching their lazily-created matrices. +GeometryWithFinders MakeDividedGeometry(const char *name) +{ + auto *manager = new TGeoManager(name, name); + auto *material = new TGeoMaterial("vacuum", 0., 0., 0.); + auto *medium = new TGeoMedium("vacuum", 1, material); + auto *top = manager->MakeBox("top", medium, 100., 100., 100.); + manager->SetTopVolume(top); + + auto *slab = manager->MakeBox("slab", medium, 20., 5., 5.); + slab->Divide("linear_slice", 1, 10, -20., 4.); + top->AddNode(slab, 1); + + auto *tube = manager->MakeTube("tube", medium, 1., 20., 10.); + tube->Divide("radial_slice", 1, 4, 1., 19. / 4.); + top->AddNode(tube, 1, new TGeoTranslation(0., 40., 0.)); + + manager->CloseGeometry(); + return {manager, slab->GetFinder(), tube->GetFinder()}; +} + +void MakeCurrent(TGeoManager *manager) +{ + gGeoManager = manager; + gGeoIdentity = static_cast(manager->GetListOfMatrices()->At(0)); +} + struct Ray { Double_t point[3]; Double_t dir[3]; @@ -186,3 +220,35 @@ TEST(Geometry, MultiThreadedNavigationMatchesSerial) delete geom; } + +TEST(Geometry, PatternMatricesBelongToOwningManager) +{ + auto geometryA = MakeDividedGeometry("pattern_owner_A"); + + // Keep A alive while constructing B. This is how applications that cache several + // managers switch away from the current geometry before creating another one. + gGeoManager = nullptr; + gGeoIdentity = nullptr; + auto geometryB = MakeDividedGeometry("pattern_owner_B"); + + // First-touch A while B is current. Matrix ownership must follow the divided volume, + // not the ambient globals. + TGeoMatrix *matrix = geometryA.linearFinder->GetMatrix(); + ASSERT_NE(matrix, nullptr); + EXPECT_GE(geometryA.manager->GetListOfMatrices()->IndexOf(matrix), 0); + EXPECT_LT(geometryB.manager->GetListOfMatrices()->IndexOf(matrix), 0); + + TGeoMatrix *identity = geometryA.radialFinder->GetMatrix(); + EXPECT_EQ(identity, geometryA.manager->GetListOfMatrices()->At(0)); + EXPECT_NE(identity, geometryB.manager->GetListOfMatrices()->At(0)); + + delete geometryB.manager; + MakeCurrent(geometryA.manager); + + // Under ASan this dereference also catches a matrix that was wrongly owned and deleted by B. + geometryA.linearFinder->cd(0); + EXPECT_EQ(geometryA.linearFinder->GetMatrix(), matrix); + EXPECT_TRUE(geometryA.radialFinder->GetMatrix()->IsIdentity()); + + delete geometryA.manager; +} From 7cf472ec50b5f6b6454672ce75a5945e61bd7a28 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:01:23 +0200 Subject: [PATCH 4/6] [geom] Reclaim shape TLS scratch data 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 once navigation using the shape has stopped. Cover cleanup and lazy rebuilding from stale TLS slots with multiple worker threads. --- geom/geom/inc/TGeoPgon.h | 30 ++++----- geom/geom/inc/TGeoXtru.h | 31 ++++----- geom/geom/src/TGeoPgon.cxx | 63 +++++++----------- geom/geom/src/TGeoXtru.cxx | 98 ++++++++++------------------ geom/test/test_thread_navigation.cxx | 72 ++++++++++++++++++++ 5 files changed, 155 insertions(+), 139 deletions(-) diff --git a/geom/geom/inc/TGeoPgon.h b/geom/geom/inc/TGeoPgon.h index dcc0badc3a976..566851cea3831 100644 --- a/geom/geom/inc/TGeoPgon.h +++ b/geom/geom/inc/TGeoPgon.h @@ -16,11 +16,13 @@ #include #include +#include +#include #include class TGeoPgon : public TGeoPcon { - static std::atomic fgInstanceCount; //! source of dense per-object indices - UInt_t fIndex{fgInstanceCount++}; //! dense index of this shape into the per-thread vector + static std::atomic fgInstanceCount; //! source of monotonic per-object indices + UInt_t fIndex{fgInstanceCount++}; //! non-reused index of this shape into the per-thread vector mutable std::atomic fGeneration{0}; //! bumped whenever the per-thread state must be rebuilt public: @@ -28,18 +30,9 @@ class TGeoPgon : public TGeoPcon { Int_t *fIntBuffer{nullptr}; //![fNedges+4] temporary int buffer array Double_t *fDblBuffer{nullptr}; //![fNedges+4] temporary double buffer array Int_t fInitGen{-1}; //! generation this slot was last initialized for - - ThreadData_t() = default; - ~ThreadData_t(); - // Owns the two scratch buffers: movable so the slot can live in a resizable vector, - // but not copyable. - ThreadData_t(ThreadData_t &&other) noexcept; - ThreadData_t &operator=(ThreadData_t &&other) noexcept; - ThreadData_t(const ThreadData_t &) = delete; - ThreadData_t &operator=(const ThreadData_t &) = delete; }; - /// Per-thread scratch buffers, owned by the calling thread and indexed by this shape. + /// Per-thread non-owning cache of scratch buffers indexed by this shape. /// Hot path: a TLS read plus an indexed load; the cold rebuild lives in InitThreadSlot(). ThreadData_t &GetThreadData() const { @@ -51,17 +44,20 @@ class TGeoPgon : public TGeoPcon { InitThreadSlot(td); return td; } - /// Invalidate the per-thread data. Each thread rebuilds its own slot lazily on next access. - void ClearThreadData() const override { fGeneration.fetch_add(1, std::memory_order_release); } - /// No-op: per-thread data is allocated lazily, so no provisioning for a fixed thread count - /// is required and any number of threads works. + /// Release object-owned scratch buffers and invalidate the non-owning TLS slots. + /// Navigation using this shape must not be active when this method is called. + void ClearThreadData() const override; + /// No-op: this shape allocates scratch data lazily for every calling thread. void CreateThreadData(Int_t) override {} protected: + struct OwnedThreadData_t; void InitThreadSlot(ThreadData_t &td) const; // data members - Int_t fNedges; // number of edges (at least one) + Int_t fNedges; // number of edges (at least one) + mutable std::vector> fOwnedData; /// #include +#include +#include #include class TGeoPolygon; class TGeoXtru : public TGeoBBox { - static std::atomic fgInstanceCount; //! source of dense per-object indices - UInt_t fIndex{fgInstanceCount++}; //! dense index of this shape into the per-thread vector + static std::atomic fgInstanceCount; //! source of monotonic per-object indices + UInt_t fIndex{fgInstanceCount++}; //! non-reused index of this shape into the per-thread vector mutable std::atomic fGeneration{0}; //! bumped whenever the per-thread state must be rebuilt mutable std::atomic fIllegalChecked{kFALSE}; //! illegal-polygon warning already emitted @@ -34,17 +36,9 @@ class TGeoXtru : public TGeoBBox { Double_t *fYc{nullptr}; //![fNvert] current Y positions for polygon vertices TGeoPolygon *fPoly{nullptr}; //! polygon defining section shape Int_t fInitGen{-1}; //! generation this slot was last initialized for - - ThreadData_t() = default; - ~ThreadData_t(); - // Owns fXc/fYc/fPoly: movable so the slot can live in a resizable vector, not copyable. - ThreadData_t(ThreadData_t &&other) noexcept; - ThreadData_t &operator=(ThreadData_t &&other) noexcept; - ThreadData_t(const ThreadData_t &) = delete; - ThreadData_t &operator=(const ThreadData_t &) = delete; }; - /// Per-thread scratch state, owned by the calling thread and indexed by this shape. + /// Per-thread non-owning cache of scratch state indexed by this shape. /// Hot path: a TLS read plus an indexed load; the cold rebuild lives in InitThreadSlot(). ThreadData_t &GetThreadData() const { @@ -56,17 +50,14 @@ class TGeoXtru : public TGeoBBox { InitThreadSlot(td); return td; } - /// Invalidate the per-thread data. Each thread rebuilds its own slot lazily on next access. - void ClearThreadData() const override - { - fGeneration.fetch_add(1, std::memory_order_release); - fIllegalChecked.store(kFALSE, std::memory_order_relaxed); - } - /// No-op: per-thread data is allocated lazily, so no provisioning for a fixed thread count - /// is required and any number of threads works. + /// Release object-owned scratch buffers and invalidate the non-owning TLS slots. + /// Navigation using this shape must not be active when this method is called. + void ClearThreadData() const override; + /// No-op: this shape allocates scratch data lazily for every calling thread. void CreateThreadData(Int_t) override {} protected: + struct OwnedThreadData_t; void InitThreadSlot(ThreadData_t &td) const; // data members @@ -79,6 +70,8 @@ class TGeoXtru : public TGeoBBox { Double_t *fScale; //[fNz] array of scale factors (for each Z) Double_t *fX0; //[fNz] array of X offsets (for each Z) Double_t *fY0; //[fNz] array of Y offsets (for each Z) + mutable std::vector> fOwnedData; /// TGeoPgon::fgInstanceCount{0}; -//////////////////////////////////////////////////////////////////////////////// -/// Destructor. - -TGeoPgon::ThreadData_t::~ThreadData_t() -{ - delete[] fIntBuffer; - delete[] fDblBuffer; -} +struct TGeoPgon::OwnedThreadData_t { + std::unique_ptr fIntBuffer; + std::unique_ptr fDblBuffer; -//////////////////////////////////////////////////////////////////////////////// -/// Move constructor. Steals the owned buffers; the moved-from slot is left empty -/// and uninitialized (fInitGen = -1). - -TGeoPgon::ThreadData_t::ThreadData_t(ThreadData_t &&other) noexcept - : fIntBuffer(other.fIntBuffer), fDblBuffer(other.fDblBuffer), fInitGen(other.fInitGen) -{ - other.fIntBuffer = nullptr; - other.fDblBuffer = nullptr; - other.fInitGen = -1; -} + explicit OwnedThreadData_t(std::size_t size) : fIntBuffer(new Int_t[size]), fDblBuffer(new Double_t[size]) {} +}; //////////////////////////////////////////////////////////////////////////////// -/// Move assignment. Releases the current buffers before stealing the source's. +/// (Re)build the per-thread scratch buffers for this shape into the given slot. +/// Cold path: runs once per (thread, shape, generation). -TGeoPgon::ThreadData_t &TGeoPgon::ThreadData_t::operator=(ThreadData_t &&other) noexcept +void TGeoPgon::InitThreadSlot(ThreadData_t &td) const { - if (this != &other) { - delete[] fIntBuffer; - delete[] fDblBuffer; - fIntBuffer = other.fIntBuffer; - fDblBuffer = other.fDblBuffer; - fInitGen = other.fInitGen; - other.fIntBuffer = nullptr; - other.fDblBuffer = nullptr; - other.fInitGen = -1; - } - return *this; + auto data = std::make_unique(fNedges + 10); + Int_t *intBuffer = data->fIntBuffer.get(); + Double_t *dblBuffer = data->fDblBuffer.get(); + + std::lock_guard guard(fOwnedDataMutex); + fOwnedData.push_back(std::move(data)); + td.fIntBuffer = intBuffer; + td.fDblBuffer = dblBuffer; + td.fInitGen = fGeneration.load(std::memory_order_acquire); } //////////////////////////////////////////////////////////////////////////////// -/// (Re)build the per-thread scratch buffers for this shape into the given slot. -/// Cold path: runs once per (thread, shape, generation). +/// Release the large scratch buffers. Navigation using this shape must not be active. -void TGeoPgon::InitThreadSlot(ThreadData_t &td) const +void TGeoPgon::ClearThreadData() const { - td = ThreadData_t{}; // release any buffers left from a previous generation - td.fIntBuffer = new Int_t[fNedges + 10]; - td.fDblBuffer = new Double_t[fNedges + 10]; - td.fInitGen = fGeneration.load(std::memory_order_acquire); + std::lock_guard guard(fOwnedDataMutex); + fOwnedData.clear(); + fGeneration.fetch_add(1, std::memory_order_release); } //////////////////////////////////////////////////////////////////////////////// @@ -944,7 +927,7 @@ Bool_t TGeoPgon::SliceCrossingIn(const Double_t *point, const Double_t *dir, Int } ipl += incseg; } // end loop Z - } // end loop phi + } // end loop phi snext = TGeoShape::Big(); return kFALSE; } diff --git a/geom/geom/src/TGeoXtru.cxx b/geom/geom/src/TGeoXtru.cxx index e829a285d881c..4233f96a6d939 100644 --- a/geom/geom/src/TGeoXtru.cxx +++ b/geom/geom/src/TGeoXtru.cxx @@ -101,64 +101,16 @@ Double_t y0, Double_t scale); #include "TGeoManager.h" #include "TGeoVolume.h" #include "TGeoPolygon.h" -#include "TROOT.h" std::atomic TGeoXtru::fgInstanceCount{0}; -//////////////////////////////////////////////////////////////////////////////// -/// Destructor. +struct TGeoXtru::OwnedThreadData_t { + std::unique_ptr fXc; + std::unique_ptr fYc; + std::unique_ptr fPoly; -TGeoXtru::ThreadData_t::~ThreadData_t() -{ - delete[] fXc; - delete[] fYc; - // fPoly is a TObject, so deleting it walks ROOT's cleanup machinery (RecursiveRemove, - // TObjArray::Delete). This slot lives in a thread_local vector and can therefore be - // destroyed at thread/process exit, possibly after ROOT's globals have been torn down, - // where that machinery reads freed state. Only delete while ROOT is still alive -- at - // shutdown the OS reclaims this small per-thread buffer anyway. The in-run rebuild path - // (move-assignment in InitThreadSlot) always runs while ROOT is up, so this guard only - // ever skips the very last teardown, never steady-state churn. - // Same "is ROOT still there" test that TDirectory and TGenericClassInfo use. - if (fPoly && ROOT::Internal::gROOTLocal) - delete fPoly; -} - -//////////////////////////////////////////////////////////////////////////////// -/// Move constructor. Steals the owned resources; the moved-from slot is left empty and -/// uninitialized (fInitGen = -1). fPoly keeps pointing at fXc/fYc, which are not relocated. - -TGeoXtru::ThreadData_t::ThreadData_t(ThreadData_t &&other) noexcept - : fSeg(other.fSeg), fIz(other.fIz), fXc(other.fXc), fYc(other.fYc), fPoly(other.fPoly), fInitGen(other.fInitGen) -{ - other.fXc = nullptr; - other.fYc = nullptr; - other.fPoly = nullptr; - other.fInitGen = -1; -} - -//////////////////////////////////////////////////////////////////////////////// -/// Move assignment. Releases the current resources before stealing the source's. - -TGeoXtru::ThreadData_t &TGeoXtru::ThreadData_t::operator=(ThreadData_t &&other) noexcept -{ - if (this != &other) { - delete[] fXc; - delete[] fYc; - delete fPoly; - fSeg = other.fSeg; - fIz = other.fIz; - fXc = other.fXc; - fYc = other.fYc; - fPoly = other.fPoly; - fInitGen = other.fInitGen; - other.fXc = nullptr; - other.fYc = nullptr; - other.fPoly = nullptr; - other.fInitGen = -1; - } - return *this; -} + explicit OwnedThreadData_t(std::size_t size) : fXc(new Double_t[size]), fYc(new Double_t[size]) {} +}; //////////////////////////////////////////////////////////////////////////////// /// (Re)build the per-thread scratch state for this shape into the given slot. @@ -166,19 +118,39 @@ TGeoXtru::ThreadData_t &TGeoXtru::ThreadData_t::operator=(ThreadData_t &&other) void TGeoXtru::InitThreadSlot(ThreadData_t &td) const { - td = ThreadData_t{}; // release anything left from a previous generation - td.fXc = new Double_t[fNvert]; - td.fYc = new Double_t[fNvert]; - memcpy(td.fXc, fX, fNvert * sizeof(Double_t)); - memcpy(td.fYc, fY, fNvert * sizeof(Double_t)); - td.fPoly = new TGeoPolygon(fNvert); - td.fPoly->SetXY(td.fXc, td.fYc); // initialize with current coordinates - td.fPoly->FinishPolygon(); + auto data = std::make_unique(fNvert); + memcpy(data->fXc.get(), fX, fNvert * sizeof(Double_t)); + memcpy(data->fYc.get(), fY, fNvert * sizeof(Double_t)); + data->fPoly = std::make_unique(fNvert); + data->fPoly->SetXY(data->fXc.get(), data->fYc.get()); + data->fPoly->FinishPolygon(); + Double_t *xc = data->fXc.get(); + Double_t *yc = data->fYc.get(); + TGeoPolygon *poly = data->fPoly.get(); + + std::lock_guard guard(fOwnedDataMutex); + fOwnedData.push_back(std::move(data)); + td.fSeg = 0; + td.fIz = 0; + td.fXc = xc; + td.fYc = yc; + td.fPoly = poly; + td.fInitGen = fGeneration.load(std::memory_order_acquire); // The polygon is identical in every thread, so report an illegal one exactly once // instead of once per thread (previously: only for thread id 0). if (td.fPoly->IsIllegalCheck() && !fIllegalChecked.exchange(kTRUE, std::memory_order_relaxed)) Error("DefinePolygon", "Shape %s of type XTRU has an illegal polygon.", GetName()); - td.fInitGen = fGeneration.load(std::memory_order_acquire); +} + +//////////////////////////////////////////////////////////////////////////////// +/// Release the large scratch buffers. Navigation using this shape must not be active. + +void TGeoXtru::ClearThreadData() const +{ + std::lock_guard guard(fOwnedDataMutex); + fOwnedData.clear(); + fGeneration.fetch_add(1, std::memory_order_release); + fIllegalChecked.store(kFALSE, std::memory_order_relaxed); } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/test/test_thread_navigation.cxx b/geom/test/test_thread_navigation.cxx index 4f02d5797ddba..59e5c6e288549 100644 --- a/geom/test/test_thread_navigation.cxx +++ b/geom/test/test_thread_navigation.cxx @@ -15,7 +15,9 @@ #include #include +#include #include +#include #include #include @@ -114,6 +116,28 @@ void MakeCurrent(TGeoManager *manager) gGeoIdentity = static_cast(manager->GetListOfMatrices()->At(0)); } +class InspectablePgon : public TGeoPgon { +public: + using TGeoPgon::TGeoPgon; + + std::size_t GetOwnedThreadDataCount() const + { + std::lock_guard guard(fOwnedDataMutex); + return fOwnedData.size(); + } +}; + +class InspectableXtru : public TGeoXtru { +public: + using TGeoXtru::TGeoXtru; + + std::size_t GetOwnedThreadDataCount() const + { + std::lock_guard guard(fOwnedDataMutex); + return fOwnedData.size(); + } +}; + struct Ray { Double_t point[3]; Double_t dir[3]; @@ -252,3 +276,51 @@ TEST(Geometry, PatternMatricesBelongToOwningManager) delete geometryA.manager; } + +TEST(Geometry, ShapeScratchDataReleasedOnClear) +{ + InspectablePgon pgon(0., 360., 64, 2); + pgon.DefineSection(0, -10., 1., 5.); + pgon.DefineSection(1, 10., 1., 5.); + + InspectableXtru xtru(2); + Double_t x[] = {-5., 5., 5., -5.}; + Double_t y[] = {-5., -5., 5., 5.}; + xtru.DefinePolygon(4, x, y); + xtru.DefineSection(0, -10.); + xtru.DefineSection(1, 10.); + + // DefineSection computes the Xtru bounding box using the main-thread slot. + pgon.ClearThreadData(); + xtru.ClearThreadData(); + + constexpr int kNThreads = 8; + std::atomic valid{true}; + std::vector threads; + threads.reserve(kNThreads); + for (int i = 0; i < kNThreads; ++i) { + threads.emplace_back([&] { + auto &pgonData = pgon.GetThreadData(); + auto &xtruData = xtru.GetThreadData(); + if (!pgonData.fIntBuffer || !pgonData.fDblBuffer || !xtruData.fXc || !xtruData.fYc || !xtruData.fPoly) + valid.store(false, std::memory_order_relaxed); + }); + } + for (auto &thread : threads) + thread.join(); + + ASSERT_TRUE(valid.load(std::memory_order_relaxed)); + EXPECT_EQ(pgon.GetOwnedThreadDataCount(), kNThreads); + EXPECT_EQ(xtru.GetOwnedThreadDataCount(), kNThreads); + + pgon.ClearThreadData(); + xtru.ClearThreadData(); + EXPECT_EQ(pgon.GetOwnedThreadDataCount(), 0u); + EXPECT_EQ(xtru.GetOwnedThreadDataCount(), 0u); + + // The main thread's stale non-owning slots must rebuild on the next access. + EXPECT_NE(pgon.GetThreadData().fIntBuffer, nullptr); + EXPECT_NE(xtru.GetThreadData().fPoly, nullptr); + EXPECT_EQ(pgon.GetOwnedThreadDataCount(), 1u); + EXPECT_EQ(xtru.GetOwnedThreadDataCount(), 1u); +} From 8759aeae8884b5aca8ad3fd1b03419ae46b01691 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:04:49 +0200 Subject: [PATCH 5/6] [geom] Document lazy TLS scope 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. --- geom/geom/inc/TGeoBoolNode.h | 6 ++++-- geom/geom/inc/TGeoPatternFinder.h | 8 ++++---- geom/geom/inc/TGeoPgon.h | 1 + geom/geom/inc/TGeoVolume.h | 5 +++-- geom/geom/inc/TGeoXtru.h | 1 + geom/geom/src/TGeoManager.cxx | 4 +++- geom/geom/src/TGeoPatternFinder.cxx | 6 +++--- 7 files changed, 19 insertions(+), 12 deletions(-) diff --git a/geom/geom/inc/TGeoBoolNode.h b/geom/geom/inc/TGeoBoolNode.h index c63b2aadb37b4..025209fcd27c8 100644 --- a/geom/geom/inc/TGeoBoolNode.h +++ b/geom/geom/inc/TGeoBoolNode.h @@ -24,8 +24,8 @@ class TGeoMatrix; class TGeoHMatrix; class TGeoBoolNode : public TObject { - static std::atomic fgInstanceCount; //! source of dense per-object indices - UInt_t fIndex{fgInstanceCount++}; //! dense index of this node into the per-thread vector + static std::atomic fgInstanceCount; //! source of monotonic per-object indices + UInt_t fIndex{fgInstanceCount++}; //! non-reused index of this node into the per-thread vector public: enum EGeoBoolType { @@ -39,6 +39,7 @@ class TGeoBoolNode : public TObject { /// Per-thread scratch state, owned by the calling thread and indexed by this node. /// Each thread owns its whole vector, so no two threads ever write the same cache line. + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; @@ -47,6 +48,7 @@ class TGeoBoolNode : public TObject { return tdata[fIndex]; } void ClearThreadData() const {} + /// No-op: this node allocates its scratch state lazily for every calling thread. void CreateThreadData(Int_t) {} private: diff --git a/geom/geom/inc/TGeoPatternFinder.h b/geom/geom/inc/TGeoPatternFinder.h index b1dbb0481a4d6..8e4d6c58457bf 100644 --- a/geom/geom/inc/TGeoPatternFinder.h +++ b/geom/geom/inc/TGeoPatternFinder.h @@ -24,8 +24,8 @@ class TGeoMatrix; /// base finder class for patterns. A pattern is specifying a division type class TGeoPatternFinder : public TObject { - static std::atomic fgInstanceCount; //! source of dense per-object indices - UInt_t fIndex{fgInstanceCount++}; //! dense index of this finder into the per-thread vector + static std::atomic fgInstanceCount; //! source of monotonic per-object indices + UInt_t fIndex{fgInstanceCount++}; //! non-reused index of this finder into the per-thread vector mutable std::atomic fGeneration{0}; //! bumped whenever the per-thread state must be rebuilt public: @@ -41,6 +41,7 @@ class TGeoPatternFinder : public TObject { /// Per-thread scratch state, owned by the calling thread and indexed by this finder. /// Hot path: a TLS read plus an indexed load; the cold rebuild lives in InitThreadSlot(). + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; @@ -54,8 +55,7 @@ class TGeoPatternFinder : public TObject { /// Invalidate the per-thread data. Each thread rebuilds its own slot lazily on next access, /// so no cross-thread reach-in is needed. void ClearThreadData() const { fGeneration.fetch_add(1, std::memory_order_release); } - /// No-op: per-thread data is allocated lazily, so no provisioning for a fixed thread count - /// is required and any number of threads works. + /// No-op: this finder allocates its scratch state lazily for every calling thread. void CreateThreadData(Int_t) {} protected: diff --git a/geom/geom/inc/TGeoPgon.h b/geom/geom/inc/TGeoPgon.h index 566851cea3831..7c26d63aa9250 100644 --- a/geom/geom/inc/TGeoPgon.h +++ b/geom/geom/inc/TGeoPgon.h @@ -34,6 +34,7 @@ class TGeoPgon : public TGeoPcon { /// Per-thread non-owning cache of scratch buffers indexed by this shape. /// Hot path: a TLS read plus an indexed load; the cold rebuild lives in InitThreadSlot(). + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; diff --git a/geom/geom/inc/TGeoVolume.h b/geom/geom/inc/TGeoVolume.h index 0eb19dc250e87..dcc2d61317095 100644 --- a/geom/geom/inc/TGeoVolume.h +++ b/geom/geom/inc/TGeoVolume.h @@ -317,8 +317,8 @@ class TGeoVolumeMulti : public TGeoVolume { //////////////////////////////////////////////////////////////////////////// class TGeoVolumeAssembly : public TGeoVolume { - static std::atomic fgInstanceCount; //! source of dense per-object indices - UInt_t fIndex{fgInstanceCount++}; //! dense index of this assembly into the per-thread vector + static std::atomic fgInstanceCount; //! source of monotonic per-object indices + UInt_t fIndex{fgInstanceCount++}; //! non-reused index of this assembly into the per-thread vector public: struct ThreadData_t { @@ -328,6 +328,7 @@ class TGeoVolumeAssembly : public TGeoVolume { /// Per-thread scratch state, owned by the calling thread and indexed by this assembly. /// Each thread owns its whole vector, so no two threads ever write the same cache line. + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; diff --git a/geom/geom/inc/TGeoXtru.h b/geom/geom/inc/TGeoXtru.h index 5b0d6f1fcdd44..eb2e21412d3a3 100644 --- a/geom/geom/inc/TGeoXtru.h +++ b/geom/geom/inc/TGeoXtru.h @@ -40,6 +40,7 @@ class TGeoXtru : public TGeoBBox { /// Per-thread non-owning cache of scratch state indexed by this shape. /// Hot path: a TLS read plus an indexed load; the cold rebuild lives in InitThreadSlot(). + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; diff --git a/geom/geom/src/TGeoManager.cxx b/geom/geom/src/TGeoManager.cxx index 4de5bc656e20d..f2a2d6c14f688 100644 --- a/geom/geom/src/TGeoManager.cxx +++ b/geom/geom/src/TGeoManager.cxx @@ -976,7 +976,9 @@ void TGeoManager::RemoveNavigator(const TGeoNavigator *nav) } //////////////////////////////////////////////////////////////////////////////// -/// Set maximum number of threads for navigation. +/// Enable multi-threaded navigation for at most `nthreads` worker threads. +/// The geometry must be closed and navigation must not be active when this method is called. +/// This enables ROOT thread safety and prepares the manager and geometry objects for concurrent navigation. void TGeoManager::SetMaxThreads(Int_t nthreads) { diff --git a/geom/geom/src/TGeoPatternFinder.cxx b/geom/geom/src/TGeoPatternFinder.cxx index 3b58bc7fe8408..882d3edc10b7e 100644 --- a/geom/geom/src/TGeoPatternFinder.cxx +++ b/geom/geom/src/TGeoPatternFinder.cxx @@ -58,9 +58,9 @@ void TGeoPatternFinder::InitThreadSlot(ThreadData_t &td) const std::lock_guard guard(sInitMutex); td.fMatrix = CreateMatrix(); } - // A generation bump only invalidates the cached division indices. The matrix stays valid and - // is deliberately reused: it is owned by the geometry manager and never released, so creating - // a fresh one here would leak one matrix per (thread, finder) on every ClearThreadData(). + // A generation bump only invalidates the cached division indices. The matrix stays valid for + // its owning manager's lifetime and is deliberately reused. The manager releases it during + // destruction; replacing it here would retain another matrix on every ClearThreadData(). td.fCurrent = -1; td.fNextIndex = -1; td.fInitGen = fGeneration.load(std::memory_order_acquire); From c9774b280fb02ddb611013c33b21f20ac7922e07 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:05:53 +0200 Subject: [PATCH 6/6] [RelNotes] Document multithreaded geometry improvements Summarize the improved multithreaded TGeo navigation and the reported ALICE benchmark. Credit Sandro Wenzel and Tristan Wenzel, with Tristan's affiliation corrected to ETHZ. --- README/ReleaseNotes/v642/index.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/README/ReleaseNotes/v642/index.md b/README/ReleaseNotes/v642/index.md index e83165d9f748d..c7e3c336ab392 100644 --- a/README/ReleaseNotes/v642/index.md +++ b/README/ReleaseNotes/v642/index.md @@ -36,6 +36,7 @@ The following people have contributed to this new version: Devajith Valaparambil Sreeramaswamy, CERN/EP-SFT,\ Vassil Vassilev, Princeton,\ Sandro Wenzel, CERN/EP-ALICE,\ + Tristan Wenzel, ETHZ,\ ## Deprecation and Removal @@ -207,6 +208,15 @@ Such file can be loaded locally in any web browser or send as attachment in emai ## Geometry +### Improved multithreaded `TGeo` navigation + +Multithreaded `TGeo` navigation is now faster and more scalable, with improved thread-local state management that avoids +false sharing, releases temporary memory during geometry cleanup, and correctly supports concurrent navigation of +multiple geometries. + +For ALICE material-budget lookup-table generation on 28 cores, these changes reduced the runtime from 139 s to 72 s +and improved scaling from 12x to 23x. + The [TGeometry](https://root.cern/doc/master/classTGeometry.html) classes (Geant 3 shapes) have been moved out of Graf3D into their own library. To link to these classes, use the cmake target `TGeometry` (preferred), `root-config --libs`, or link with `-lTGeometry`. When ROOT is configured with `-Dgeom=Off`, these classes are now off as well.