From 0fa9c087ba6d73d0d7de5d72fac816c70cbc0229 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 8 Aug 2024 16:59:56 +0100 Subject: [PATCH] SceneAlgo : `Fix history()` performance regression This gives us the following pleasing performance improvement : ``` testHistoryDiamondPerformance (GafferSceneTest.SceneAlgoTest.SceneAlgoTest) : was 9.11s now 0.00s (100% reduction) ``` As noted in the comment, this does mean that we're now returning a reduced history, but we don't anticipate this being a problem in practice. Clients such as `attributeHistory()` aim to whittle the graph down to a single path relevant to a particular attribute, and our HistoryWindow only shows a single linear history anyway. The histories we're returning now are a closer match for the ones we returned prior to #4568, while retaining the main benefit of that PR - the evaluations of thing we're not trying to track are still pulled from the cache if possible. Fixes #5199 --- Changes.md | 1 + src/GafferScene/SceneAlgo.cpp | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/Changes.md b/Changes.md index 63fbf48f3d3..057dd721ac2 100644 --- a/Changes.md +++ b/Changes.md @@ -14,6 +14,7 @@ Improvements - Blue : Proxy - Red : Guide - Catalogue : Added a handle for controlling the relative sizes of the listing and image property widgets. +- RenderPassEditor, LightEditor : Improved update performance for certain graph configurations, by optimising `SceneAlgo::history()` (#5199). Fixes ----- diff --git a/src/GafferScene/SceneAlgo.cpp b/src/GafferScene/SceneAlgo.cpp index bcbfccf46d1..fdf0eeef5ed 100644 --- a/src/GafferScene/SceneAlgo.cpp +++ b/src/GafferScene/SceneAlgo.cpp @@ -479,6 +479,13 @@ class CapturingMonitor : public Monitor // before we were made active via `Monitor::Scope`. m_rootProcesses.push_back( std::move( capturedProcess ) ); } + + // Remember that we've monitored this process so that we dont force + // its monitoring again. + + IECore::MurmurHash h = process->context()->hash(); + h.append( reinterpret_cast( p ) ); + m_monitoredSet.insert( h ); } void processFinished( const Process *process ) override @@ -494,7 +501,22 @@ class CapturingMonitor : public Monitor bool forceMonitoring( const Plug *plug, const IECore::InternedString &processType ) override { - return processType == g_hashProcessType && shouldCapture( plug ); + if( processType != g_hashProcessType || !shouldCapture( plug ) ) + { + return false; + } + + // Don't force the monitoring of a process we've monitored already. This does + // mean we throw away diamond dependencies in the process graph, but it is essential + // for performance in some cases - see `testHistoryDiamondPerformance()` for example. + /// \todo Potentially we could use the hash to find the previously captured process, + /// and instance that into our process graph. This would require clients of `History` + /// to be updated to handle such topologies efficiently by tracking previously visited + /// items. It may also be of fairly low value, since typically our goal is to find the + /// first relevant path through the graph to present to the user. + IECore::MurmurHash h = Context::current()->hash(); + h.append( reinterpret_cast( plug ) ); + return !m_monitoredSet.count( h ); } private : @@ -509,15 +531,18 @@ class CapturingMonitor : public Monitor } using Mutex = tbb::spin_mutex; - - Mutex m_mutex; - using ProcessOrScope = boost::variant>; using ProcessMap = boost::unordered_map; + const IECore::InternedString m_scenePlugChildName; + + // Protects `m_processMap` and `m_rootProcesses`. + /// \todo Perhaps they should be concurrent containers instead? + Mutex m_mutex; ProcessMap m_processMap; CapturedProcess::PtrVector m_rootProcesses; - IECore::InternedString m_scenePlugChildName; + + tbb::concurrent_unordered_set m_monitoredSet; };