Improve _GatherDependencies performance by pre-caching path lookup in parallel #1815
Conversation
|
Filed as internal issue #USD-7294 |
pxr/usdImaging/usdImaging/delegate.h
Outdated
| typedef TfHashMap<SdfPath, SdfPathVector, SdfPath::Hash> _DependenciesCacheMap; | ||
| _DependenciesCacheMap _dependenciesCacheMap; | ||
|
|
||
| void _GatherDependenciesCache(SdfPath const &subtree, |
There was a problem hiding this comment.
Can we change this to _CacheDependencies or _PrecacheDependencies? It makes it more clear what the function is doing.
The pixar code convention is that parameters a function writes out to have to be a pointer, not a reference. Since the SdfPathVector is owned by the _dependenciesCacheMap, you could change this to SdfPathVector**, or else make the return type "const SdfPathVector&" instead of void...
| @@ -659,7 +659,13 @@ class UsdImagingDelegate : public HdSceneDelegate, public TfWeakBase { | |||
| _DependencyMap _dependencyInfo; | |||
|
|
|||
| void _GatherDependencies(SdfPath const& subtree, | |||
| SdfPathVector *affectedCachePaths); | |||
There was a problem hiding this comment.
As with _GatherDependenciesCache, our code convention is to not have data written out to reference parameters.
pxr/usdImaging/usdImaging/delegate.h
Outdated
| SdfPathVector& affectedCachePaths); | ||
|
|
||
| typedef TfHashMap<SdfPath, SdfPathVector, SdfPath::Hash> _DependenciesCacheMap; | ||
| _DependenciesCacheMap _dependenciesCacheMap; |
There was a problem hiding this comment.
I'm worried about this being confused with _dependencyInfo with the current name. Some other names that could work: _flattenedDependencyCache, _recursiveDependencyCache, etc.
| // Pre-cache dependencies in parallel | ||
| WorkDispatcher resyncPathsCacheDispatcher; | ||
| for (const auto& usdPath: sortedUsdPathsToResync) { | ||
| auto& affectedCachePaths = _dependenciesCacheMap[usdPath]; |
There was a problem hiding this comment.
Building "sortedUsdPathsToResync" will be expensive. You can delete the loop above by running through "usdPathsToResync", doing the GetPrimPath/GetParentPath/etc, and then replacing this line with:
auto pair = _dependenciesCacheMap.insert(usdPath);
if (pair.second) { continue; /* already inserted */ }
auto& affectedCachePaths = pair->first.second;
There was a problem hiding this comment.
Note also that you might want to add caching for "usdPathsToUpdate" as well. The paths to resync are a result of USD composition/type changes, whereas updates reflect property value changes; both call _GatherDependencies.
|
Thanks for the review, I will update as soon as I can. |
…ion, rename variable for better readability
|
Hi @tcauchois , sorry for the delay, I have addressed the feedback in commit b8a78ea . |
… dependency gathering PR #1815 introduced a (persistent) flattened dependency cache map in UsdImagingDelegate to improve performance when a large hierarchy of prims are resync'd or updated. This resulted in a few bugs stemming from the cache and the existing _dependencyInfo map being out-of-sync. This change addresses these issues: - Use a local cache instead of a persistent one that is populated in parallel. This lets us salvage a lot of the performance benefits and avoids the complexity of maintaining an up-to-date and nimble cache. - Clear the cache when any changes are made to the _dependencyInfo map from resyncing the Usd prim or subtree. - Add a simple test case that would've caught the bug from the PR. (Internal change: 2258076)
Description of Change(s)
This PR is to improve the performance for
UsdImagingDelegate::SetTimeby pre-caching path dependencies in parallel, in our production shot, the time dropped from 1.2 min to be 10.x second in total.Fixes Issue(s)