@@ -437,8 +437,6 @@ class CapturingMonitor : public Monitor
437437 {
438438 const Plug *p = process->plug ();
439439
440- CapturedProcess::Ptr capturedProcess;
441- ProcessOrScope entry;
442440 if ( !shouldCapture ( p ) )
443441 {
444442 // Parents may spawn other processes in support of the requested plug. This is one
@@ -450,39 +448,44 @@ class CapturingMonitor : public Monitor
450448 // order of the stack is preserved - if this happens out of order, the stack will be
451449 // corrupted, and weird crashes will happen. But as long as it is created in
452450 // processStarted, and released in processFinished, everything should line up.
453- entry = std::make_unique<Monitor::Scope>( this , false );
454- }
455- else
456- {
457- capturedProcess = std::make_unique<CapturedProcess>();
458- capturedProcess->type = process->type ();
459- capturedProcess->plug = p;
460- capturedProcess->destinationPlug = process->destinationPlug ();
461- capturedProcess->context = new Context ( *process->context (), /* omitCanceller = */ true );
462- entry = capturedProcess.get ();
451+ Mutex::scoped_lock lock ( m_mutex );
452+ m_processMap[process] = std::make_unique<Monitor::Scope>( this , false );
453+ return ;
463454 }
464455
456+ // Capture this process.
457+
458+ CapturedProcess::Ptr capturedProcess = std::make_unique<CapturedProcess>();
459+ capturedProcess->type = process->type ();
460+ capturedProcess->plug = p;
461+ capturedProcess->destinationPlug = process->destinationPlug ();
462+ capturedProcess->context = new Context ( *process->context (), /* omitCanceller = */ true );
463+
465464 Mutex::scoped_lock lock ( m_mutex );
466- m_processMap[process] = std::move ( entry );
465+ m_processMap[process] = capturedProcess. get ( );
467466
468- if ( capturedProcess )
467+ ProcessMap::const_iterator it = m_processMap.find ( process->parent () );
468+ if ( it != m_processMap.end () )
469469 {
470- ProcessMap::const_iterator it = m_processMap. find ( process-> parent () );
471- if ( it != m_processMap. end () )
470+ CapturedProcess * const * parent = std::get_if<CapturedProcess *>( &it-> second );
471+ if ( parent && *parent )
472472 {
473- CapturedProcess * const * parent = std::get_if<CapturedProcess *>( &it->second );
474- if ( parent && *parent )
475- {
476- (*parent)->children .push_back ( std::move ( capturedProcess ) );
477- }
478- }
479- else
480- {
481- // Either `process->parent()` was null, or was started
482- // before we were made active via `Monitor::Scope`.
483- m_rootProcesses.push_back ( std::move ( capturedProcess ) );
473+ (*parent)->children .push_back ( std::move ( capturedProcess ) );
484474 }
485475 }
476+ else
477+ {
478+ // Either `process->parent()` was null, or was started
479+ // before we were made active via `Monitor::Scope`.
480+ m_rootProcesses.push_back ( std::move ( capturedProcess ) );
481+ }
482+
483+ // Remember that we've monitored this process so that we dont force
484+ // its monitoring again.
485+
486+ IECore::MurmurHash h = process->context ()->hash ();
487+ h.append ( reinterpret_cast <intptr_t >( p ) );
488+ m_monitoredSet.insert ( h );
486489 }
487490
488491 void processFinished ( const Process *process ) override
@@ -498,12 +501,22 @@ class CapturingMonitor : public Monitor
498501
499502 bool forceMonitoring ( const Plug *plug, const IECore::InternedString &processType ) override
500503 {
501- if ( processType == g_hashProcessType && shouldCapture ( plug ) )
504+ if ( processType != g_hashProcessType || ! shouldCapture ( plug ) )
502505 {
503- return true ;
506+ return false ;
504507 }
505508
506- return false ;
509+ // Don't force the monitoring of a process we've monitored already. This does
510+ // mean we throw away diamond dependencies in the process graph, but it is essential
511+ // for performance in some cases - see `testHistoryDiamondPerformance()` for example.
512+ // / \todo Potentially we could use the hash to find the previously captured process,
513+ // / and instance that into our process graph. This would require clients of `History`
514+ // / to be updated to handle such topologies efficiently by tracking previously visited
515+ // / items. It may also be of fairly low value, since typically our goal is to find the
516+ // / first relevant path through the graph to present to the user.
517+ IECore::MurmurHash h = Context::current ()->hash ();
518+ h.append ( reinterpret_cast <intptr_t >( plug ) );
519+ return !m_monitoredSet.count ( h );
507520 }
508521
509522 private :
@@ -518,15 +531,18 @@ class CapturingMonitor : public Monitor
518531 }
519532
520533 using Mutex = tbb::spin_mutex;
521-
522- Mutex m_mutex;
523-
524534 using ProcessOrScope = std::variant<CapturedProcess *, std::unique_ptr<Monitor::Scope>>;
525535 using ProcessMap = boost::unordered_map<const Process *, ProcessOrScope>;
526536
537+ const IECore::InternedString m_scenePlugChildName;
538+
539+ // Protects `m_processMap` and `m_rootProcesses`.
540+ // / \todo Perhaps they should be concurrent containers instead?
541+ Mutex m_mutex;
527542 ProcessMap m_processMap;
528543 CapturedProcess::PtrVector m_rootProcesses;
529- IECore::InternedString m_scenePlugChildName;
544+
545+ tbb::concurrent_unordered_set<IECore::MurmurHash> m_monitoredSet;
530546
531547};
532548
0 commit comments