From 71b56b2a3c1116dbe8dcb11d2fd90f7f6647e6d6 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 14:45:21 +0200 Subject: [PATCH 1/4] [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/TGeoMatrix.h | 7 +++ geom/geom/inc/TGeoPatternFinder.h | 2 + geom/geom/src/TGeoMatrix.cxx | 26 ++++++++--- geom/geom/src/TGeoPatternFinder.cxx | 58 ++++++++++++++---------- geom/test/test_thread_navigation.cxx | 66 ++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 27 deletions(-) diff --git a/geom/geom/inc/TGeoMatrix.h b/geom/geom/inc/TGeoMatrix.h index 56f65f8572356..0bcbd2f7ec17c 100644 --- a/geom/geom/inc/TGeoMatrix.h +++ b/geom/geom/inc/TGeoMatrix.h @@ -29,6 +29,7 @@ const Double_t kIdentityMatrix[3 * 3] = {1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, const Double_t kUnitScale[3] = {1.0, 1.0, 1.0}; class TGeoHMatrix; +class TGeoManager; //////////////////////////////////////////////////////////////////////////// // // @@ -96,6 +97,9 @@ class TGeoMatrix : public TNamed { virtual void ReflectY(Bool_t leftside, Bool_t rotonly = kFALSE); virtual void ReflectZ(Bool_t leftside, Bool_t rotonly = kFALSE); virtual void RegisterYourself(); + /// Register this matrix with an explicit owning geometry manager. + /// \param manager Geometry manager that takes ownership of the matrix. + void RegisterYourself(TGeoManager *manager); void SetDefaultName(); virtual void SetDx(Double_t) {} virtual void SetDy(Double_t) {} @@ -341,6 +345,9 @@ class TGeoCombiTrans : public TGeoMatrix { TGeoMatrix *MakeClone() const override; void Multiply(const TGeoMatrix *right); void RegisterYourself() override; + /// Register this matrix and its rotation with an explicit owning geometry manager. + /// \param manager Geometry manager that takes ownership of the transformations. + void RegisterYourself(TGeoManager *manager); void RotateX(Double_t angle) override; void RotateY(Double_t angle) override; void RotateZ(Double_t angle) override; diff --git a/geom/geom/inc/TGeoPatternFinder.h b/geom/geom/inc/TGeoPatternFinder.h index f9c508311d86b..0e13c4a856b5e 100644 --- a/geom/geom/inc/TGeoPatternFinder.h +++ b/geom/geom/inc/TGeoPatternFinder.h @@ -60,6 +60,8 @@ class TGeoPatternFinder : public TObject { protected: void InitThreadSlot(ThreadData_t &td) const; + TGeoManager *GetOwnerManager() const { return fVolume ? fVolume->GetGeoManager() : nullptr; } + TGeoMatrix *GetOwnerIdentity() const; enum EGeoPatternFlags { kPatternReflected = BIT(14), kPatternSpacedOut = BIT(15) }; Double_t fStep; // division step length diff --git a/geom/geom/src/TGeoMatrix.cxx b/geom/geom/src/TGeoMatrix.cxx index 86739568a28d8..1493ffbc073c1 100644 --- a/geom/geom/src/TGeoMatrix.cxx +++ b/geom/geom/src/TGeoMatrix.cxx @@ -539,12 +539,20 @@ void TGeoMatrix::ReflectZ(Bool_t, Bool_t) {} void TGeoMatrix::RegisterYourself() { - if (!gGeoManager) { + RegisterYourself(gGeoManager); +} + +//////////////////////////////////////////////////////////////////////////////// +/// Register the matrix in the given manager, which will become the owner. + +void TGeoMatrix::RegisterYourself(TGeoManager *manager) +{ + if (!manager) { Warning("RegisterYourself", "cannot register without geometry"); return; } if (!IsRegistered()) { - gGeoManager->RegisterMatrix(this); + manager->RegisterMatrix(this); SetBit(kGeoRegistered); } } @@ -1940,9 +1948,17 @@ void TGeoCombiTrans::Multiply(const TGeoMatrix *right) void TGeoCombiTrans::RegisterYourself() { - TGeoMatrix::RegisterYourself(); - if (fRotation && fRotation->IsRotation()) - fRotation->RegisterYourself(); + RegisterYourself(gGeoManager); +} + +//////////////////////////////////////////////////////////////////////////////// +/// Register the matrix and its rotation in the given manager. + +void TGeoCombiTrans::RegisterYourself(TGeoManager *manager) +{ + TGeoMatrix::RegisterYourself(manager); + if (manager && fRotation && fRotation->IsRotation()) + fRotation->RegisterYourself(manager); } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/geom/src/TGeoPatternFinder.cxx b/geom/geom/src/TGeoPatternFinder.cxx index 5649fe5552ee0..5b4957b4bb321 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,15 @@ 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; +} + //////////////////////////////////////////////////////////////////////////////// /// Default constructor @@ -271,11 +285,11 @@ TGeoMatrix *TGeoPatternX::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -465,11 +479,11 @@ TGeoMatrix *TGeoPatternY::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -655,11 +669,11 @@ TGeoMatrix *TGeoPatternZ::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -918,11 +932,11 @@ TGeoMatrix *TGeoPatternParaX::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1096,11 +1110,11 @@ TGeoMatrix *TGeoPatternParaY::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1278,11 +1292,11 @@ TGeoMatrix *TGeoPatternParaZ::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1467,11 +1481,11 @@ TGeoMatrix *TGeoPatternTrapZ::CreateMatrix() const { if (!IsReflected()) { TGeoMatrix *matrix = new TGeoTranslation(0., 0., 0.); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoCombiTrans *combi = new TGeoCombiTrans(); - combi->RegisterYourself(); + combi->RegisterYourself(GetOwnerManager()); combi->ReflectZ(kTRUE); combi->ReflectZ(kFALSE); return combi; @@ -1631,7 +1645,7 @@ void TGeoPatternCylR::SavePrimitive(std::ostream &out, Option_t * /*option*/ /*= TGeoMatrix *TGeoPatternCylR::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// @@ -1827,11 +1841,11 @@ TGeoMatrix *TGeoPatternCylPhi::CreateMatrix() const { if (!IsReflected()) { TGeoRotation *matrix = new TGeoRotation(); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoRotation *rot = new TGeoRotation(); - rot->RegisterYourself(); + rot->RegisterYourself(GetOwnerManager()); rot->ReflectZ(kTRUE); rot->ReflectZ(kFALSE); return rot; @@ -1949,7 +1963,7 @@ void TGeoPatternSphR::SavePrimitive(std::ostream &out, Option_t * /*option*/ /*= TGeoMatrix *TGeoPatternSphR::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// @@ -2062,7 +2076,7 @@ void TGeoPatternSphTheta::SavePrimitive(std::ostream &out, Option_t * /*option*/ TGeoMatrix *TGeoPatternSphTheta::CreateMatrix() const { - return gGeoIdentity; + return GetOwnerIdentity(); } //////////////////////////////////////////////////////////////////////////////// @@ -2241,11 +2255,11 @@ TGeoMatrix *TGeoPatternSphPhi::CreateMatrix() const { if (!IsReflected()) { TGeoRotation *matrix = new TGeoRotation(); - matrix->RegisterYourself(); + matrix->RegisterYourself(GetOwnerManager()); return matrix; } TGeoRotation *rot = new TGeoRotation(); - rot->RegisterYourself(); + rot->RegisterYourself(GetOwnerManager()); rot->ReflectZ(kTRUE); rot->ReflectZ(kFALSE); return rot; @@ -2342,7 +2356,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 57968650dc9dacae97223ecb797886feac46f3ad Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:01:23 +0200 Subject: [PATCH 2/4] [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 after navigation is quiescent. 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..47aa558a65b73 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 be quiescent before 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 be quiescent before 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 must already be quiescent. -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..f7e59fca1373f 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 must already be quiescent. + +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 7700c62eaa2c8b51238dedb288dcc5cf98a7cc47 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:04:49 +0200 Subject: [PATCH 3/4] [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 | 13 ++++++++----- 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, 23 insertions(+), 13 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 0e13c4a856b5e..6e732a7af6f93 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: @@ -63,7 +63,10 @@ class TGeoPatternFinder : public TObject { TGeoManager *GetOwnerManager() const { return fVolume ? fVolume->GetGeoManager() : nullptr; } TGeoMatrix *GetOwnerIdentity() 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/inc/TGeoPgon.h b/geom/geom/inc/TGeoPgon.h index 47aa558a65b73..1bd64d3c6f18a 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 6bb88e4c76b8e..cb15d0d4d1de3 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..53d031a120c7e 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. +/// This is still required to make navigator registration and manager state thread-safe; +/// lazy scratch-data allocation does not replace this setup. void TGeoManager::SetMaxThreads(Int_t nthreads) { diff --git a/geom/geom/src/TGeoPatternFinder.cxx b/geom/geom/src/TGeoPatternFinder.cxx index 5b4957b4bb321..3c401ab7a160e 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 eee13d1acd9373c879a8951aa75eb011a8a6f672 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:05:53 +0200 Subject: [PATCH 4/4] [RelNotes] Credit geometry contributors 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. --- README/ReleaseNotes/v642/index.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README/ReleaseNotes/v642/index.md b/README/ReleaseNotes/v642/index.md index e83165d9f748d..d6468d1fe8c31 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, CERN/EP-ALICE,\ ## Deprecation and Removal