From 9378f9ebb7053b51c22ff40dfc705b1186d45756 Mon Sep 17 00:00:00 2001 From: tcauchois Date: Sun, 9 Jun 2024 21:56:40 -0700 Subject: [PATCH] [hd] Cleanup of SceneIndexAdapterSceneDelegate::_PrimCacheEntry. This replaces the (atomic, map) pair used to track cached primvar descriptors with a single atomic shared ptr to an array, following the caching pattern we use elsewhere (e.g. flattening scene index). This has two effects: - The atomic logic is simpler and more robust when it's not split across two objects. - The datastructures aren't inlined into _PrimCacheEntry, making _PrimCacheEntry much smaller and improving cache efficiency a bit. (Internal change: 2330018) --- .../hd/sceneIndexAdapterSceneDelegate.cpp | 138 ++++++++++-------- .../hd/sceneIndexAdapterSceneDelegate.h | 39 ++--- 2 files changed, 92 insertions(+), 85 deletions(-) diff --git a/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp b/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp index db0b7bc2a0..01cc218cc8 100644 --- a/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp +++ b/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp @@ -250,12 +250,10 @@ HdSceneIndexAdapterSceneDelegate::_PrimAdded( // Make sure prim type is up-to-date and clear caches. entry.primType = primType; - entry.primvarDescriptors.clear(); - entry.primvarDescriptorsState.store( - _PrimCacheEntry::ReadStateUnread); - entry.extCmpPrimvarDescriptors.clear(); - entry.extCmpPrimvarDescriptorsState.store( - _PrimCacheEntry::ReadStateUnread); + std::atomic_store(&(entry.primvarDescriptors), + std::shared_ptr<_PrimCacheEntry::PrimvarDescriptorsArray>()); + std::atomic_store(&(entry.extCmpPrimvarDescriptors), + std::shared_ptr<_PrimCacheEntry::ExtCmpPrimvarDescriptorsArray>()); } else { _primCache[indexPath].primType = primType; } @@ -444,16 +442,14 @@ HdSceneIndexAdapterSceneDelegate::PrimsDirtied( if (entry.dirtyLocators.Intersects( HdPrimvarsSchema::GetDefaultLocator())) { - it->second.primvarDescriptors.clear(); - it->second.primvarDescriptorsState.store( - _PrimCacheEntry::ReadStateUnread); + std::atomic_store(&(it->second.primvarDescriptors), + std::shared_ptr<_PrimCacheEntry::PrimvarDescriptorsArray>()); } if (entry.dirtyLocators.Intersects( HdExtComputationPrimvarsSchema::GetDefaultLocator())) { - it->second.extCmpPrimvarDescriptors.clear(); - it->second.extCmpPrimvarDescriptorsState.store( - _PrimCacheEntry::ReadStateUnread); + std::atomic_store(&(it->second.extCmpPrimvarDescriptors), + std::shared_ptr<_PrimCacheEntry::ExtCmpPrimvarDescriptorsArray>()); } } } @@ -1666,28 +1662,47 @@ HdSceneIndexAdapterSceneDelegate::GetPrimvarDescriptors( { TRACE_FUNCTION(); HF_MALLOC_TAG_FUNCTION(); - HdPrimvarDescriptorVector result; _PrimCacheTable::iterator it = _primCache.find(id); if (it == _primCache.end()) { + HdPrimvarDescriptorVector result; return result; } - if (it->second.primvarDescriptorsState.load() == - _PrimCacheEntry::ReadStateRead) { - return it->second.primvarDescriptors[interpolation]; + std::shared_ptr<_PrimCacheEntry::PrimvarDescriptorsArray> expected = + std::atomic_load(&(it->second.primvarDescriptors)); + if (expected) { + return (*expected)[interpolation]; } HdSceneIndexPrim prim = _GetInputPrim(id); - if (!prim.dataSource) { - it->second.primvarDescriptorsState.store( - _PrimCacheEntry::ReadStateRead); - return result; + std::shared_ptr<_PrimCacheEntry::PrimvarDescriptorsArray> desired = + _ComputePrimvarDescriptors(prim.dataSource); + + // Since multiple threads may arrive here at once, we only let the first one + // write, and later threads discard their version. + if (std::atomic_compare_exchange_strong( + &(it->second.primvarDescriptors), + &expected, + desired)) { + return (*desired)[interpolation]; + } else { + return (*expected)[interpolation]; + } +} + +std::shared_ptr +HdSceneIndexAdapterSceneDelegate::_ComputePrimvarDescriptors( + const HdContainerDataSourceHandle &primDataSource) +{ + if (!primDataSource) { + return nullptr; } - std::map descriptors; + _PrimCacheEntry::PrimvarDescriptorsArray descriptors; + if (HdPrimvarsSchema primvars = - HdPrimvarsSchema::GetFromParent(prim.dataSource)) { + HdPrimvarsSchema::GetFromParent(primDataSource)) { for (const TfToken &name : primvars.GetPrimvarNames()) { HdPrimvarSchema primvar = primvars.GetPrimvar(name); if (!primvar) { @@ -1706,6 +1721,10 @@ HdSceneIndexAdapterSceneDelegate::GetPrimvarDescriptors( HdInterpolation interpolation = Hd_InterpolationAsEnum(interpolationToken); + if (interpolation >= HdInterpolationCount) { + continue; + } + TfToken roleToken; if (HdTokenDataSourceHandle roleDataSource = primvar.GetRole()) { @@ -1719,17 +1738,8 @@ HdSceneIndexAdapterSceneDelegate::GetPrimvarDescriptors( } } - _PrimCacheEntry::ReadState current = _PrimCacheEntry::ReadStateUnread; - if (it->second.primvarDescriptorsState.compare_exchange_strong( - current, _PrimCacheEntry::ReadStateReading)) { - it->second.primvarDescriptors = std::move(descriptors); - it->second.primvarDescriptorsState.store( - _PrimCacheEntry::ReadStateRead); - - return it->second.primvarDescriptors[interpolation]; - } - - return descriptors[interpolation]; + return std::make_shared<_PrimCacheEntry::PrimvarDescriptorsArray>( + std::move(descriptors)); } HdExtComputationPrimvarDescriptorVector @@ -1738,29 +1748,47 @@ HdSceneIndexAdapterSceneDelegate::GetExtComputationPrimvarDescriptors( { TRACE_FUNCTION(); HF_MALLOC_TAG_FUNCTION(); - HdExtComputationPrimvarDescriptorVector result; _PrimCacheTable::iterator it = _primCache.find(id); if (it == _primCache.end()) { + HdExtComputationPrimvarDescriptorVector result; return result; } - if (it->second.extCmpPrimvarDescriptorsState.load() == - _PrimCacheEntry::ReadStateRead) { - return it->second.extCmpPrimvarDescriptors[interpolation]; + std::shared_ptr<_PrimCacheEntry::ExtCmpPrimvarDescriptorsArray> expected = + std::atomic_load(&(it->second.extCmpPrimvarDescriptors)); + if (expected) { + return (*expected)[interpolation]; } HdSceneIndexPrim prim = _GetInputPrim(id); - if (!prim.dataSource) { - it->second.extCmpPrimvarDescriptorsState.store( - _PrimCacheEntry::ReadStateRead); - return result; + std::shared_ptr<_PrimCacheEntry::ExtCmpPrimvarDescriptorsArray> desired = + _ComputeExtCmpPrimvarDescriptors(prim.dataSource); + + // Since multiple threads may arrive here at once, we only let the first one + // write, and later threads discard their version. + if (std::atomic_compare_exchange_strong( + (&it->second.extCmpPrimvarDescriptors), + &expected, + desired)) { + return (*desired)[interpolation]; + } else { + return (*expected)[interpolation]; + } +} + +std::shared_ptr +HdSceneIndexAdapterSceneDelegate::_ComputeExtCmpPrimvarDescriptors( + const HdContainerDataSourceHandle &primDataSource) +{ + if (!primDataSource) { + return nullptr; } - std::map - descriptors; + _PrimCacheEntry::ExtCmpPrimvarDescriptorsArray descriptors; + if (HdExtComputationPrimvarsSchema primvars = - HdExtComputationPrimvarsSchema::GetFromParent(prim.dataSource)) { + HdExtComputationPrimvarsSchema::GetFromParent(primDataSource)) { for (const TfToken &name : primvars.GetExtComputationPrimvarNames()) { HdExtComputationPrimvarSchema primvar = primvars.GetPrimvar(name); if (!primvar) { @@ -1778,6 +1806,10 @@ HdSceneIndexAdapterSceneDelegate::GetExtComputationPrimvarDescriptors( HdInterpolation interpolation = Hd_InterpolationAsEnum(interpolationToken); + if (interpolation >= HdInterpolationCount) { + continue; + } + TfToken roleToken; if (HdTokenDataSourceHandle roleDataSource = primvar.GetRole()) { @@ -1809,22 +1841,8 @@ HdSceneIndexAdapterSceneDelegate::GetExtComputationPrimvarDescriptors( } } - if (it->second.extCmpPrimvarDescriptorsState.load() == - _PrimCacheEntry::ReadStateUnread) { - it->second.extCmpPrimvarDescriptorsState.store( - _PrimCacheEntry::ReadStateReading); - it->second.extCmpPrimvarDescriptors = std::move(descriptors); - it->second.extCmpPrimvarDescriptorsState.store( - _PrimCacheEntry::ReadStateRead); - } else { - // if someone is in the process of filling the entry, just - // return our value instead of trying to assign - return descriptors[interpolation]; - } - - return it->second.extCmpPrimvarDescriptors[interpolation]; - - + return std::make_shared<_PrimCacheEntry::ExtCmpPrimvarDescriptorsArray>( + std::move(descriptors)); } VtValue diff --git a/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.h b/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.h index f851cd04d3..ab112c11c3 100644 --- a/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.h +++ b/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.h @@ -247,38 +247,27 @@ class HdSceneIndexAdapterSceneDelegate struct _PrimCacheEntry { - _PrimCacheEntry() - : primvarDescriptorsState(ReadStateUnread) - , extCmpPrimvarDescriptorsState(ReadStateUnread) - {} - - _PrimCacheEntry(const _PrimCacheEntry &rhs) - { - primType = rhs.primType; - primvarDescriptorsState.store(rhs.primvarDescriptorsState.load()); - extCmpPrimvarDescriptorsState.store( - rhs.extCmpPrimvarDescriptorsState.load()); - } - TfToken primType; - enum ReadState : unsigned char { - ReadStateUnread = 0, - ReadStateReading, - ReadStateRead, - }; - - std::atomic primvarDescriptorsState; - std::atomic extCmpPrimvarDescriptorsState; - std::map - primvarDescriptors; - std::map - extCmpPrimvarDescriptors; + using PrimvarDescriptorsArray = + std::array; + std::shared_ptr primvarDescriptors; + using ExtCmpPrimvarDescriptorsArray = + std::array; + std::shared_ptr extCmpPrimvarDescriptors; }; using _PrimCacheTable = SdfPathTable<_PrimCacheEntry>; _PrimCacheTable _primCache; + std::shared_ptr<_PrimCacheEntry::PrimvarDescriptorsArray> + _ComputePrimvarDescriptors( + const HdContainerDataSourceHandle &primDataSource); + std::shared_ptr<_PrimCacheEntry::ExtCmpPrimvarDescriptorsArray> + _ComputeExtCmpPrimvarDescriptors( + const HdContainerDataSourceHandle &primDataSource); + bool _sceneDelegatesBuilt; std::vector _sceneDelegates;