From d6d85f1bf9d1136fddc7ed20e31d62c44b295108 Mon Sep 17 00:00:00 2001 From: Andrei Gheata Date: Mon, 27 Jul 2026 15:53:46 +0200 Subject: [PATCH] [geom] Fix stale thread-local navigators Invalidate cached navigator and thread-ID state across geometry manager lifetimes while keeping navigator selection local to each worker thread. Synchronize shared navigator-map lookups and add regression coverage for concurrent navigator creation and manager recreation after parallel overlap checks. --- geom/geom/src/TGeoManager.cxx | 106 ++++++++++++++++++------ geom/geom/src/TGeoNode.cxx | 10 +-- geom/geom/src/TGeoVolume.cxx | 10 +-- geom/test/CMakeLists.txt | 6 ++ geom/test/test_manager_lifetime.cxx | 123 ++++++++++++++++++++++++++++ 5 files changed, 218 insertions(+), 37 deletions(-) create mode 100644 geom/test/test_manager_lifetime.cxx diff --git a/geom/geom/src/TGeoManager.cxx b/geom/geom/src/TGeoManager.cxx index 3bb85ca845789..4de5bc656e20d 100644 --- a/geom/geom/src/TGeoManager.cxx +++ b/geom/geom/src/TGeoManager.cxx @@ -236,6 +236,7 @@ in order to enhance rays. \image html geom_random2.jpg */ +#include #include #include #include @@ -305,6 +306,33 @@ TGeoManager::EDefaultUnits TGeoManager::fgDefaultUnits = TGeoManager::kRootUnits TGeoManager::ThreadsMap_t *TGeoManager::fgThreadId = nullptr; static Bool_t gGeometryLocked = kFALSE; +namespace { + +struct TGeoManagerThreadState { + const TGeoManager *fManager = nullptr; + TGeoNavigator *fNavigator = nullptr; + ULong64_t fNavigatorGeneration = 0; + Int_t fThreadId = -1; + ULong64_t fThreadIdGeneration = 0; +}; + +TGeoManagerThreadState &GetGeoManagerThreadState() +{ + TTHREAD_TLS(TGeoManagerThreadState) state; + return state; +} + +// Thread-local navigator pointers and thread ordinals cannot be reset by the thread deleting a manager. +// Advancing this generation on destructive/global state transitions makes every thread refresh on its next access. +std::atomic gGeoManagerThreadStateGeneration{1}; + +void InvalidateGeoManagerThreadState() +{ + gGeoManagerThreadStateGeneration.fetch_add(1, std::memory_order_release); +} + +} // namespace + //////////////////////////////////////////////////////////////////////////////// /// Default constructor. @@ -807,8 +835,13 @@ TGeoNavigator *TGeoManager::AddNavigator() TGeoNavigator *nav = array->AddNavigator(); if (fClosed) nav->GetCache()->BuildInfoBranch(); - if (fMultiThread) + if (fMultiThread) { + auto &state = GetGeoManagerThreadState(); + state.fManager = this; + state.fNavigator = nav; + state.fNavigatorGeneration = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire); fgMutex.unlock(); + } return nav; } @@ -817,20 +850,27 @@ TGeoNavigator *TGeoManager::AddNavigator() TGeoNavigator *TGeoManager::GetCurrentNavigator() const { - TTHREAD_TLS(TGeoNavigator *) tnav = nullptr; if (!fMultiThread) return fCurrentNavigator; - TGeoNavigator *nav = tnav; // TTHREAD_TLS_GET(TGeoNavigator*,tnav); - if (nav) - return nav; + auto &state = GetGeoManagerThreadState(); + const auto generation = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire); + if (state.fNavigator && state.fManager == this && state.fNavigatorGeneration == generation) + return state.fNavigator; + + std::lock_guard lock(fgMutex); std::thread::id threadId = std::this_thread::get_id(); NavigatorsMap_t::const_iterator it = fNavigators.find(threadId); - if (it == fNavigators.end()) + if (it == fNavigators.end()) { + state.fManager = this; + state.fNavigator = nullptr; + state.fNavigatorGeneration = generation; return nullptr; + } TGeoNavigatorArray *array = it->second; - nav = array->GetCurrentNavigator(); - tnav = nav; // TTHREAD_TLS_SET(TGeoNavigator*,tnav,nav); - return nav; + state.fManager = this; + state.fNavigator = array->GetCurrentNavigator(); + state.fNavigatorGeneration = generation; + return state.fNavigator; } //////////////////////////////////////////////////////////////////////////////// @@ -838,6 +878,9 @@ TGeoNavigator *TGeoManager::GetCurrentNavigator() const TGeoNavigatorArray *TGeoManager::GetListOfNavigators() const { + std::unique_lock lock(fgMutex, std::defer_lock); + if (fMultiThread) + lock.lock(); std::thread::id threadId = std::this_thread::get_id(); NavigatorsMap_t::const_iterator it = fNavigators.find(threadId); if (it == fNavigators.end()) @@ -851,6 +894,9 @@ TGeoNavigatorArray *TGeoManager::GetListOfNavigators() const Bool_t TGeoManager::SetCurrentNavigator(Int_t index) { + std::unique_lock lock(fgMutex, std::defer_lock); + if (fMultiThread) + lock.lock(); std::thread::id threadId = std::this_thread::get_id(); NavigatorsMap_t::const_iterator it = fNavigators.find(threadId); if (it == fNavigators.end()) { @@ -865,8 +911,14 @@ Bool_t TGeoManager::SetCurrentNavigator(Int_t index) std::cout << " thread id: " << threadId << std::endl; return kFALSE; } - if (!fMultiThread) + if (fMultiThread) { + auto &state = GetGeoManagerThreadState(); + state.fManager = this; + state.fNavigator = nav; + state.fNavigatorGeneration = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire); + } else { fCurrentNavigator = nav; + } return kTRUE; } @@ -883,6 +935,7 @@ void TGeoManager::SetNavigatorsLock(Bool_t flag) void TGeoManager::ClearNavigators() { + InvalidateGeoManagerThreadState(); if (fMultiThread) fgMutex.lock(); TGeoNavigatorArray *arr = nullptr; @@ -907,6 +960,7 @@ void TGeoManager::RemoveNavigator(const TGeoNavigator *nav) TGeoNavigatorArray *arr = (*it).second; if (arr) { if ((TGeoNavigator *)arr->Remove((TObject *)nav)) { + InvalidateGeoManagerThreadState(); delete nav; if (!arr->GetEntries()) fNavigators.erase(it); @@ -944,6 +998,7 @@ void TGeoManager::SetMaxThreads(Int_t nthreads) ClearThreadsMap(); ClearThreadData(); } + InvalidateGeoManagerThreadState(); fMaxThreads = nthreads + 1; if (fMaxThreads > 0) { fMultiThread = kTRUE; @@ -986,6 +1041,7 @@ void TGeoManager::CreateThreadData() const void TGeoManager::ClearThreadsMap() { + InvalidateGeoManagerThreadState(); if (gGeoManager && !gGeoManager->IsMultiThread()) return; fgMutex.lock(); @@ -1001,23 +1057,27 @@ void TGeoManager::ClearThreadsMap() Int_t TGeoManager::ThreadId() { - TTHREAD_TLS(Int_t) tid = -1; - Int_t ttid = tid; // TTHREAD_TLS_GET(Int_t,tid); - if (ttid > -1) - return ttid; - if (gGeoManager && !gGeoManager->IsMultiThread()) + auto &state = GetGeoManagerThreadState(); + const auto generation = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire); + if (state.fThreadId > -1 && state.fThreadIdGeneration == generation) + return state.fThreadId; + if (gGeoManager && !gGeoManager->IsMultiThread()) { + state.fThreadId = 0; + state.fThreadIdGeneration = generation; return 0; + } std::thread::id threadId = std::this_thread::get_id(); + std::lock_guard lock(fgMutex); TGeoManager::ThreadsMapIt_t it = fgThreadId->find(threadId); - if (it != fgThreadId->end()) - return it->second; - // Map needs to be updated. - fgMutex.lock(); + if (it != fgThreadId->end()) { + state.fThreadId = it->second; + state.fThreadIdGeneration = generation; + return state.fThreadId; + } (*fgThreadId)[threadId] = fgNumThreads; - tid = fgNumThreads; // TTHREAD_TLS_SET(Int_t,tid,fgNumThreads); - ttid = fgNumThreads++; - fgMutex.unlock(); - return ttid; + state.fThreadId = fgNumThreads++; + state.fThreadIdGeneration = generation; + return state.fThreadId; } //////////////////////////////////////////////////////////////////////////////// diff --git a/geom/geom/src/TGeoNode.cxx b/geom/geom/src/TGeoNode.cxx index 3b094519d0282..f2e3bc0ba48e9 100644 --- a/geom/geom/src/TGeoNode.cxx +++ b/geom/geom/src/TGeoNode.cxx @@ -323,13 +323,9 @@ void TGeoNode::CheckOverlaps(Double_t ovlp, Option_t *option) timer.Start(); pool.Foreach( [&](const std::pair &range) { - // one-time init per OS thread - static thread_local bool navInit = false; - if (!navInit) { - if (!geom->GetCurrentNavigator()) - geom->AddNavigator(); - navInit = true; - } + // Make sure this manager has a navigator for the current worker. + if (!geom->GetCurrentNavigator()) + geom->AddNavigator(); std::vector local; local.reserve(32); diff --git a/geom/geom/src/TGeoVolume.cxx b/geom/geom/src/TGeoVolume.cxx index 79972b22bb0ec..d640852acaa50 100644 --- a/geom/geom/src/TGeoVolume.cxx +++ b/geom/geom/src/TGeoVolume.cxx @@ -680,13 +680,9 @@ void TGeoVolume::CheckOverlaps(Double_t ovlp, Option_t *option) timer.Start(); pool.Foreach( [&](const std::pair &range) { - // one-time init per OS thread - static thread_local bool navInit = false; - if (!navInit) { - if (!geom->GetCurrentNavigator()) - geom->AddNavigator(); - navInit = true; - } + // Make sure this manager has a navigator for the current worker. + if (!geom->GetCurrentNavigator()) + geom->AddNavigator(); std::vector local; local.reserve(32); diff --git a/geom/test/CMakeLists.txt b/geom/test/CMakeLists.txt index 1101f8aa8ad69..afcf07a6fa6a6 100644 --- a/geom/test/CMakeLists.txt +++ b/geom/test/CMakeLists.txt @@ -20,3 +20,9 @@ endif() ROOT_ADD_GTEST(tessellated test_tessellated.cxx LIBRARIES Geom) + +if(imt) + ROOT_ADD_GTEST(manager_lifetime + test_manager_lifetime.cxx + LIBRARIES Geom GeomChecker Imt) +endif() diff --git a/geom/test/test_manager_lifetime.cxx b/geom/test/test_manager_lifetime.cxx new file mode 100644 index 0000000000000..9df4ba6b180a8 --- /dev/null +++ b/geom/test/test_manager_lifetime.cxx @@ -0,0 +1,123 @@ +#include +#include +#include + +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace { + +TGeoManager *MakeGeometry(int iteration) +{ + TGeoManager *geom = nullptr; + // TGeoManager::Init deletes the existing gGeoManager, as happens when the + // interpreted macro is executed again from the main thread. + if (gGeoManager) { + ROOT_EXPECT_WARNING_PARTIAL(geom = + new TGeoManager(TString::Format("world_%d", iteration), "navigator lifetime test"), + "TGeoManager::Init", "Deleting previous geometry:"); + } else { + geom = new TGeoManager(TString::Format("world_%d", iteration), "navigator lifetime test"); + } + auto *material = new TGeoMaterial("Vacuum", 0., 0., 0.); + auto *medium = new TGeoMedium("Vacuum", 1, material); + auto *top = geom->MakeBox("top", medium, 10., 10., 10.); + auto *sphere = geom->MakeSphere("sphere", medium, 0., 5.); + top->AddNode(sphere, 1); + geom->SetTopVolume(top); + geom->CloseGeometry(); + return geom; +} + +class ImplicitMTGuard { +public: + explicit ImplicitMTGuard(unsigned int numThreads) { ROOT::EnableImplicitMT(numThreads); } + ~ImplicitMTGuard() { ROOT::DisableImplicitMT(); } +}; + +} // namespace + +TEST(TGeoManager, ConcurrentNavigatorsStayThreadLocal) +{ + constexpr unsigned int numThreads = 4; + constexpr int numNavigators = 16; + + auto *geom = MakeGeometry(0); + geom->SetMaxThreads(numThreads); + + // Start all workers together so navigator creation and lookup overlap. + std::atomic numReady{0}; + std::atomic start{false}; + std::atomic failed{false}; + std::vector workers; + workers.reserve(numThreads); + + for (unsigned int i = 0; i < numThreads; ++i) { + workers.emplace_back([&] { + numReady.fetch_add(1, std::memory_order_release); + while (!start.load(std::memory_order_acquire)) + std::this_thread::yield(); + + // Every worker owns an independent navigator array. Adding a navigator + // here must update only this worker's current-navigator cache, even as + // the other workers add their navigators concurrently. + TGeoNavigator *first = nullptr; + TGeoNavigator *current = nullptr; + for (int j = 0; j < numNavigators; ++j) { + current = geom->AddNavigator(); + if (!first) + first = current; + for (int check = 0; check < 32; ++check) { + if (geom->GetCurrentNavigator() != current) + failed.store(true, std::memory_order_relaxed); + } + } + + // Selecting another navigator must likewise affect only this worker. + if (!geom->SetCurrentNavigator(0) || geom->GetCurrentNavigator() != first) + failed.store(true, std::memory_order_relaxed); + }); + } + + while (numReady.load(std::memory_order_acquire) != numThreads) + std::this_thread::yield(); + start.store(true, std::memory_order_release); + + for (auto &worker : workers) + worker.join(); + + EXPECT_FALSE(failed.load(std::memory_order_relaxed)); + delete gGeoManager; +} + +TEST(TGeoManager, RecreateAfterParallelOverlapCheck) +{ + ImplicitMTGuard imtGuard(2); + + // CheckOverlaps populates navigator caches in persistent IMT worker threads. + // On the next iteration, MakeGeometry deletes the old manager on the main + // thread and creates a new one, while the worker threads remain alive. + for (int iteration = 0; iteration < 10; ++iteration) { + auto *geom = MakeGeometry(iteration); + + // Foreach waits for all workers before returning. Reused workers must + // reject a navigator cached for the previous manager generation and book + // a navigator belonging to this manager. + geom->CheckOverlaps(0.001); + + // The calling thread must also resolve the current manager's navigator. + ASSERT_TRUE(geom->IsMultiThread()); + ASSERT_NE(geom->GetListOfNavigators(), nullptr); + ASSERT_EQ(geom->GetCurrentNavigator(), geom->GetListOfNavigators()->GetCurrentNavigator()); + EXPECT_EQ(geom->GetStackLevel(), 0); + } + + delete gGeoManager; +}