From 36cdfba40e48d83e3b083a94241b6c4470f19006 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 8 Aug 2024 11:04:11 +0100 Subject: [PATCH 1/4] CapturingMonitor : Simplify control flow --- src/GafferScene/SceneAlgo.cpp | 57 +++++++++++++++-------------------- 1 file changed, 24 insertions(+), 33 deletions(-) diff --git a/src/GafferScene/SceneAlgo.cpp b/src/GafferScene/SceneAlgo.cpp index 24e30a44015..bcbfccf46d1 100644 --- a/src/GafferScene/SceneAlgo.cpp +++ b/src/GafferScene/SceneAlgo.cpp @@ -437,8 +437,6 @@ class CapturingMonitor : public Monitor { const Plug *p = process->plug(); - CapturedProcess::Ptr capturedProcess; - ProcessOrScope entry; if( !shouldCapture( p ) ) { // Parents may spawn other processes in support of the requested plug. This is one @@ -450,39 +448,37 @@ class CapturingMonitor : public Monitor // order of the stack is preserved - if this happens out of order, the stack will be // corrupted, and weird crashes will happen. But as long as it is created in // processStarted, and released in processFinished, everything should line up. - entry = std::make_unique( this, false ); - } - else - { - capturedProcess = std::make_unique(); - capturedProcess->type = process->type(); - capturedProcess->plug = p; - capturedProcess->destinationPlug = process->destinationPlug(); - capturedProcess->context = new Context( *process->context(), /* omitCanceller = */ true ); - entry = capturedProcess.get(); + Mutex::scoped_lock lock( m_mutex ); + m_processMap[process] = std::make_unique( this, false ); + return; } + // Capture this process. + + CapturedProcess::Ptr capturedProcess = std::make_unique(); + capturedProcess->type = process->type(); + capturedProcess->plug = p; + capturedProcess->destinationPlug = process->destinationPlug(); + capturedProcess->context = new Context( *process->context(), /* omitCanceller = */ true ); + Mutex::scoped_lock lock( m_mutex ); - m_processMap[process] = std::move( entry ); + m_processMap[process] = capturedProcess.get(); - if( capturedProcess ) + ProcessMap::const_iterator it = m_processMap.find( process->parent() ); + if( it != m_processMap.end() ) { - ProcessMap::const_iterator it = m_processMap.find( process->parent() ); - if( it != m_processMap.end() ) - { - CapturedProcess * const * parent = boost::get< CapturedProcess* >( &it->second ); - if( parent && *parent ) - { - (*parent)->children.push_back( std::move( capturedProcess ) ); - } - } - else + CapturedProcess * const * parent = boost::get< CapturedProcess* >( &it->second ); + if( parent && *parent ) { - // Either `process->parent()` was null, or was started - // before we were made active via `Monitor::Scope`. - m_rootProcesses.push_back( std::move( capturedProcess ) ); + (*parent)->children.push_back( std::move( capturedProcess ) ); } } + else + { + // Either `process->parent()` was null, or was started + // before we were made active via `Monitor::Scope`. + m_rootProcesses.push_back( std::move( capturedProcess ) ); + } } void processFinished( const Process *process ) override @@ -498,12 +494,7 @@ class CapturingMonitor : public Monitor bool forceMonitoring( const Plug *plug, const IECore::InternedString &processType ) override { - if( processType == g_hashProcessType && shouldCapture( plug ) ) - { - return true; - } - - return false; + return processType == g_hashProcessType && shouldCapture( plug ); } private : From e99ade35db60fa64293eec1fbaa4d1ef6aee8971 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 8 Aug 2024 13:27:45 +0100 Subject: [PATCH 2/4] ValuePlug : Fix the plug being passed to `Process::forceMonitoring()` --- Changes.md | 1 + src/Gaffer/ValuePlug.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Changes.md b/Changes.md index 7efa8756890..63fbf48f3d3 100644 --- a/Changes.md +++ b/Changes.md @@ -26,6 +26,7 @@ Fixes - Fixed bug which allowed locked Catalogues to be edited. - Fixed NodeEditor update when the first image is added or the last image is removed. - NameWidget : Fixed bug which allowed plugs on locked nodes to be renamed. +- ValuePlug : Fixed the plug passed to `Monitor::forceMonitoring()`. Previously `Process::destinationPlug()` was being passed instead of `Process::plug()`. API --- diff --git a/src/Gaffer/ValuePlug.cpp b/src/Gaffer/ValuePlug.cpp index 5e5c224877a..fe036e133e9 100644 --- a/src/Gaffer/ValuePlug.cpp +++ b/src/Gaffer/ValuePlug.cpp @@ -241,7 +241,7 @@ class ValuePlug::HashProcess : public Process // Then get our hash. We do this using this `acquireHash()` functor so that // we can repeat the process for `Checked` mode. - const bool forceMonitoring = Process::forceMonitoring( threadState, plug, staticType ); + const bool forceMonitoring = Process::forceMonitoring( threadState, p, staticType ); auto acquireHash = [&]( const HashCacheKey &cacheKey ) { From f22c10b19e49f32091ce1872913bbdf0725eea49 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 8 Aug 2024 16:45:37 +0100 Subject: [PATCH 3/4] SceneAlgoTest : Expose performance problem in `history()` This test takes the best part of 10 seconds per run, and models problems we saw in two separate production environments (not using Loops there, but just a series of manually made diamond connections). --- python/GafferSceneTest/SceneAlgoTest.py | 40 +++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/python/GafferSceneTest/SceneAlgoTest.py b/python/GafferSceneTest/SceneAlgoTest.py index 86df510ada6..c1c29248d43 100644 --- a/python/GafferSceneTest/SceneAlgoTest.py +++ b/python/GafferSceneTest/SceneAlgoTest.py @@ -1946,6 +1946,46 @@ def assertNoCanceller( history ) : assertNoCanceller( history ) + @GafferTest.TestRunner.PerformanceTestMethod() + def testHistoryDiamondPerformance( self ) : + + # A series of diamonds where every iteration reads the output of the + # previous iteration twice and then feeds into the next iteration. + # + # o + # / \ + # o o + # \ / + # o + # / \ + # o o + # \ / + # o + # / \ + # ... + # \ / + # o + # + # Without caching, this leads to an explosion of paths through the + # graph, and can lead to poor performance in `history()`. + + plane = GafferScene.Plane() + + loop = Gaffer.Loop() + loop.setup( plane["out"] ) + loop["in"].setInput( plane["out"] ) + + copyOptions = GafferScene.CopyOptions() + copyOptions["in"].setInput( loop["previous"] ) + copyOptions["source"].setInput( loop["previous"] ) + copyOptions["options"].setValue( "*" ) + + loop["next"].setInput( copyOptions["out"] ) + loop["iterations"].setValue( 20 ) + + with GafferTest.TestRunner.PerformanceScope() : + GafferScene.SceneAlgo.history( loop["out"]["globals"] ) + def testLinkingQueries( self ) : # Everything linked to `defaultLights` via the default value for the attribute. From 0fa9c087ba6d73d0d7de5d72fac816c70cbc0229 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 8 Aug 2024 16:59:56 +0100 Subject: [PATCH 4/4] 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; };