[geom] Reduce thread false sharing and improve multithreaded TGeo navigation

[sawenzel]

This PR improves the multithreaded performance of TGeoNavigator by reducing the overhead of thread-local scratch data access and eliminating false sharing between threads.

The changes are the result of profile-guided optimization while profiling the parallel material budget scan in ALICE (see AliceO2Group/AliceO2#15641).

On a 28-core machine, the material budget LUT generation improves from a 12× to a 23× speedup (139 s → 72 s). Since the optimizations are in the geometry navigation infrastructure itself, they should benefit any application relying on multithreaded geometry algorithms.

This is the result of a summer internship of @trwenz on whose behalf the PR is opened.

[sawenzel]

This graph demonstrates the improved scaling. It was measured on an older version of ROOT but it translates to the master version.

[github-actions]

Test Results

    23 files      23 suites   3d 18h 56m 28s ⏱️
 3 883 tests  3 883 ✅ 0 💤 0 ❌
79 983 runs  79 983 ✅ 0 💤 0 ❌

Results for commit 9ebb0b7.

[silverweed]

Are all these vectors simply never freed? Probably not a problem for most threads, but is this ever called from the main thread? If so the memory may be hoarded indefinitely for the entire process duration, no?

[agheata]

The vector is freed when its owning thread exits, but not when the corresponding geometry object is destroyed. Thus, the main thread and persistent worker threads retain their high-water mark for their lifetime. More importantly, TGeoPgon and TGeoXtru TLS entries own additional heap buffers that are never revisited after destruction because indices are monotonically increasing and never reused. This can become large, so it needs explicit reclamation, e.g. TLS entries caching non-owning pointers to object-owned, cache-aligned thread data, which can be released when the geometry is deleted.

[agheata]

Thanks for this excellent optimization work, with substantial improvements that I could verify. It would be good to have some changes only around lifetime and ownership aspects introduced by the new TLS design:

• TGeoPgon and TGeoXtru TLS entries retain their heap allocations until the worker thread exits, rather than releasing them when the corresponding geometry object is destroyed. The fast TLS lookup can be preserved by making these entries non-owning caches while the geometry object owns and releases the allocations (see suggestions inline)
• Lazy TGeoPatternFinder matrix creation relies on the current active gGeoManager/gGeoIdentity. With multiple managers, a finder belonging to A can therefore cache a matrix owned by B. Creation and registration should explicitly use fVolume->GetGeoManager()
• Monotonic indices create a process-lifetime TLS high-water mark. This may be acceptable if repeated geometry churn is out of scope, but the intended lifecycle should be documented.
• The statement about no longer requiring SetMaxThreads() should be limited to scratch-data provisioning, since navigator registration still uses it to enable thread safety.

[agheata]

The vector is freed when its owning thread exits, but not when the corresponding geometry object is destroyed. Thus, the main thread and persistent worker threads retain their high-water mark for their lifetime. More importantly, TGeoPgon and TGeoXtru TLS entries own additional heap buffers that are never revisited after destruction because indices are monotonically increasing and never reused. This can become large, so it needs explicit reclamation, e.g. TLS entries caching non-owning pointers to object-owned, cache-aligned thread data, which can be released when the geometry is deleted.

[agheata]

Lazy creation makes the gGeoManager unreliable here. CreateMatrix() implementations call RegisterYourself(), which registers with whichever manager is globally current. To correct this, you should obtain the owner from fVolume->GetGeoManager() and pass it explicitly into matrix creation/registration. Otherwise, first-touching a finder from geometry A while B is current registers A’s matrix with B, leaving A’s TLS slot dangling after B is deleted.

I would change the interface to:
virtual TGeoMatrix *CreateMatrix(TGeoManager *manager) const = 0;

For implementations of CreateMatrix, instead of RegisterYourself(), you can use manager->RegisterMatrix(). Otherwise, the cleanest minimal API addition is:

void TGeoMatrix::RegisterYourself(TGeoManager *manager);
keeping the existing overload:

void TGeoMatrix::RegisterYourself()
{
   RegisterYourself(gGeoManager);
}

Then, call this as:

TGeoManager *manager = fVolume ? fVolume->GetGeoManager() : nullptr;
if (!manager) {
   Error("InitThreadSlot", "Pattern finder has no owning geometry manager");
   return;
}
td.fMatrix = CreateMatrix(manager);

[agheata]

Identity matrices such as those coming from TGeoPatternCylR::CreateMatrix() must also come from the owner manager, not the gGeoIdentity

[agheata]

Small clarification: TGeoManager::RegisterMatrix()/TGeoBuilder::RegisterMatrix() does not set the matrix’s kGeoRegistered bit. If registration is performed directly, that state must also be preserved. An overload RegisterYourself(TGeoManager *) is probably safer because it keeps registration and the bit update together.

[agheata]

The matrix is not “never released”: TGeoManager::~TGeoManager() deletes fMatrices. Retaining this pointer is valid only while the finder’s owning manager is alive, and only if the matrix was registered with that manager. With ambient registration, deleting an unrelated current manager can invalidate this slot.

[agheata]

This TLS vector owns the Pgon buffers indirectly through ThreadData_t. Object destruction cannot reach these entries. Since fIndex is never reused, a destroyed shape’s slot is never visited again, so its arrays remain allocated until the thread exits. I would rather make the TLS entry a non-owning {generation, ThreadData_t *} cache and let the Pgon own the cache-aligned ThreadData_t instances created on cold first-touch.

[agheata]

Incrementing the generation only releases the old buffers if this same object is subsequently accessed and reaches InitThreadSlot(). From the destructor that cannot happen. ClearThreadData() should delete the object-owned thread-data blocks when not needed, and then increment the generation.

[agheata]

With monotonic indices, many TGeoPgon and multiple geometries this can use quite some memory. I would make TLS entries non-owning and let each TGeoPgon/TGeoXtru own the buffers created for its worker threads, register a newly created buffer with the object only on cold first-touch, and delete those registered buffers in ClearThreadData() once navigation of that geometry is over.

Conceptually:

struct ThreadData_t {
   Int_t *fIntBuffer{nullptr};       // non-owning
   Double_t *fDblBuffer{nullptr};    // non-owning
   Int_t fInitGen{-1};
};

mutable std::mutex fOwnedDataMutex;
mutable std::vector<std::unique_ptr<OwnedBuffers>> fOwnedData;

On first touch:

auto buffers = std::make_unique<OwnedBuffers>(fNedges);
td.fIntBuffer = buffers->fIntBuffer.get();
td.fDblBuffer = buffers->fDblBuffer.get();

std::lock_guard guard(fOwnedDataMutex);
fOwnedData.push_back(std::move(buffers));

On cleanup:

void TGeoPgon::ClearThreadData() const
{
   // Requires navigation of this shape to be finished.
   std::lock_guard guard(fOwnedDataMutex);
   fOwnedData.clear();
   fGeneration.fetch_add(1, std::memory_order_release);
}

The lock is taken only during first-touch and cleanup; the navigation hot path remains the PR’s TLS indexed access. The TLS vectors still retain their relatively small high-water capacity, but the potentially large Pgon/Xtru allocations are released with the geometry.

[agheata]

Please add a multiple-manager regression test for the lazy matrix lifetime: keep geometries A and B alive, make B current, first-touch a pattern finder belonging to A, and verify that its matrix is registered with A rather than B. Then delete B and use A again. This reproduces the current ownership bug and is detected as a use-after-free under ASan.

[agheata]

Small clarification: TGeoManager::RegisterMatrix()/TGeoBuilder::RegisterMatrix() does not set the matrix’s kGeoRegistered bit. If registration is performed directly, that state must also be preserved. An overload RegisterYourself(TGeoManager *) is probably safer because it keeps registration and the bit update together.

[sawenzel]

Fixed the merge conflict with master. As discussed privately with @agheata, this PR can be amended by maintainers in order to arrive at best possible solution for TGeo and to address the suggestions directly.