Skip to content

Commit

Permalink
SceneAlgo : Fix history() performance regression
Browse files Browse the repository at this point in the history
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
  • Loading branch information
johnhaddon committed Aug 9, 2024
1 parent f22c10b commit 06f307a
Showing 1 changed file with 30 additions and 5 deletions.
35 changes: 30 additions & 5 deletions src/GafferScene/SceneAlgo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<intptr_t>( p ) );
m_monitoredSet.insert( h );
}

void processFinished( const Process *process ) override
Expand All @@ -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<intptr_t>( plug ) );
return !m_monitoredSet.count( h );
}

private :
Expand All @@ -509,15 +531,18 @@ class CapturingMonitor : public Monitor
}

using Mutex = tbb::spin_mutex;

Mutex m_mutex;

using ProcessOrScope = boost::variant<CapturedProcess *, std::unique_ptr< Monitor::Scope>>;
using ProcessMap = boost::unordered_map<const Process *, ProcessOrScope>;

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<IECore::MurmurHash> m_monitoredSet;

};

Expand Down

0 comments on commit 06f307a

Please sign in to comment.