From 9d080245088e3ad5f68fa51c1da9e85863abfc40 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/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 da702ffd72d5fe4ae4ed41d07a8d0dde4a7ec04c 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 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 eade158331a58ed659b0404c94b7e7bcdfa48873 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 | 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 c474590f7610329924bb77757b8cde3b13ff9ab9 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Tue, 1 Sep 2026 15:05:53 +0200 Subject: [PATCH 4/4] [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.