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. 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 f9c508311d86b..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,14 +55,19 @@ 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: 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/inc/TGeoPgon.h b/geom/geom/inc/TGeoPgon.h index dcc0badc3a976..7c26d63aa9250 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,19 +30,11 @@ 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(). + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; @@ -51,17 +45,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; /// 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 9e3db2243ca09..eb2e21412d3a3 100644 --- a/geom/geom/inc/TGeoXtru.h +++ b/geom/geom/inc/TGeoXtru.h @@ -16,13 +16,15 @@ #include #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,18 +36,11 @@ 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(). + /// The vector retains its high-water size until the owning thread exits. ThreadData_t &GetThreadData() const { thread_local std::vector tdata; @@ -56,17 +51,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 +71,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; /// 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 @@ -53,14 +58,42 @@ 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); } +//////////////////////////////////////////////////////////////////////////////// +/// 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/geom/src/TGeoPgon.cxx b/geom/geom/src/TGeoPgon.cxx index 05130b9b9d14e..bdfe0fe0ab05b 100644 --- a/geom/geom/src/TGeoPgon.cxx +++ b/geom/geom/src/TGeoPgon.cxx @@ -68,55 +68,38 @@ polygons, between `phi1` and `phi1+dphi.` std::atomic 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 d01691526de78..59e5c6e288549 100644 --- a/geom/test/test_thread_navigation.cxx +++ b/geom/test/test_thread_navigation.cxx @@ -8,13 +8,16 @@ #include #include #include +#include #include #include #include #include #include +#include #include +#include #include #include @@ -80,6 +83,61 @@ 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)); +} + +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]; @@ -186,3 +244,83 @@ 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; +} + +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); +}