From 612f17b232538e2b5daece6ff07c85b819329a1b Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 11 Oct 2023 16:33:54 +0100 Subject: [PATCH] ValuePlug : Avoid unnecessary reference counting When `getObjectValue()` returns `m_staticValue`, the caller does not need to own a reference, because it is guaranteed to be kept alive by the plug itself. The caller only needs to own a reference to computed values, which may not be cached, or may be evicted from the cache on a secondard thread. This yields the following performance improvements : - testHashCacheOverhead (GafferTest.ValuePlugTest.ValuePlugTest) : was 1.15s now 1.05s (9% reduction) - testStaticNumericValuePerformance (GafferTest.ValuePlugTest.ValuePlugTest) : was 0.76s now 0.01s (99% reduction) - testStaticObjectValuePerformance (GafferTest.ValuePlugTest.ValuePlugTest) : was 0.78s now 0.72s (8% reduction) - testStaticStringValuePerformance (GafferTest.ValuePlugTest.ValuePlugTest) : was 0.95s now 0.01s (99% reduction) --- include/Gaffer/NumericPlug.inl | 3 +- include/Gaffer/TypedObjectPlug.inl | 12 ++++++- include/Gaffer/TypedPlug.inl | 3 +- include/Gaffer/ValuePlug.h | 13 +++++--- include/Gaffer/ValuePlug.inl | 26 +++++++++++---- python/GafferTest/ValuePlugTest.py | 18 +++++++++++ src/Gaffer/StringPlug.cpp | 6 ++-- src/Gaffer/ValuePlug.cpp | 44 +++++++++++++++----------- src/GafferImage/AtomicFormatPlug.cpp | 3 +- src/GafferTestModule/ValuePlugTest.cpp | 5 +++ 10 files changed, 98 insertions(+), 35 deletions(-) diff --git a/include/Gaffer/NumericPlug.inl b/include/Gaffer/NumericPlug.inl index 9abd8016249..4f46bc17131 100644 --- a/include/Gaffer/NumericPlug.inl +++ b/include/Gaffer/NumericPlug.inl @@ -42,7 +42,8 @@ namespace Gaffer template inline T NumericPlug::getValue( const IECore::MurmurHash *precomputedHash ) const { - return getObjectValue( precomputedHash )->readable(); + IECore::ConstObjectPtr owner; + return getObjectValue( owner, precomputedHash )->readable(); } } // namespace Gaffer diff --git a/include/Gaffer/TypedObjectPlug.inl b/include/Gaffer/TypedObjectPlug.inl index 6975e5cb872..609c919f93e 100644 --- a/include/Gaffer/TypedObjectPlug.inl +++ b/include/Gaffer/TypedObjectPlug.inl @@ -42,7 +42,17 @@ namespace Gaffer template inline typename TypedObjectPlug::ConstValuePtr TypedObjectPlug::getValue( const IECore::MurmurHash *precomputedHash ) const { - return getObjectValue( precomputedHash ); + IECore::ConstObjectPtr owner; + const ValueType *value = getObjectValue( owner, precomputedHash ); + if( owner ) + { + // Avoid unnecessary reference count manipulations. + return boost::static_pointer_cast( std::move( owner ) ); + } + else + { + return ConstValuePtr( value ); + } } } // namespace Gaffer diff --git a/include/Gaffer/TypedPlug.inl b/include/Gaffer/TypedPlug.inl index 0fc9033ae71..05341225481 100644 --- a/include/Gaffer/TypedPlug.inl +++ b/include/Gaffer/TypedPlug.inl @@ -42,7 +42,8 @@ namespace Gaffer template inline T TypedPlug::getValue( const IECore::MurmurHash *precomputedHash ) const { - return getObjectValue( precomputedHash )->readable(); + IECore::ConstObjectPtr owner; + return getObjectValue( owner, precomputedHash )->readable(); } } // namespace Gaffer diff --git a/include/Gaffer/ValuePlug.h b/include/Gaffer/ValuePlug.h index 4913e88e953..f66c5e097f2 100644 --- a/include/Gaffer/ValuePlug.h +++ b/include/Gaffer/ValuePlug.h @@ -250,10 +250,10 @@ class GAFFER_API ValuePlug : public Plug /// objects with each query - this allows it to support the calculation /// of values in different contexts and on different threads. /// - /// The value is returned via a reference counted pointer, as - /// following return from getObjectValue(), it is possible that nothing - /// else references the value - the value could have come from the cache - /// and then have been immediately removed by another thread. + /// The value is returned directly via a raw pointer, allowing us to omit + /// reference counting for the common case where the plug owns its own static + /// (non-computed) value. In cases where the value will be computed, a + /// a reference must be taken, so `owner` is assigned to keep the value alive. /// /// If a precomputed hash is available it may be passed to avoid computing /// it again unnecessarily. @@ -262,6 +262,9 @@ class GAFFER_API ValuePlug : public Plug /// so use with care. The hash must be the direct result of `ValuePlug::hash()`, /// so this feature is not suitable for use in classes that override that method. template + const T *getObjectValue( IECore::ConstObjectPtr &owner, const IECore::MurmurHash *precomputedHash = nullptr ) const; + /// \deprecated + template boost::intrusive_ptr getObjectValue( const IECore::MurmurHash *precomputedHash = nullptr ) const; /// Should be called by derived classes when they wish to set the plug /// value - the value is referenced directly (not copied) and so must @@ -279,6 +282,8 @@ class GAFFER_API ValuePlug : public Plug class ComputeProcess; class SetValueAction; + const IECore::Object *getValueInternal( IECore::ConstObjectPtr &owner, const IECore::MurmurHash *precomputedHash = nullptr ) const; + /// \deprecated IECore::ConstObjectPtr getValueInternal( const IECore::MurmurHash *precomputedHash = nullptr ) const; void setValueInternal( IECore::ConstObjectPtr value, bool propagateDirtiness ); void childAddedOrRemoved(); diff --git a/include/Gaffer/ValuePlug.inl b/include/Gaffer/ValuePlug.inl index 8e78d1b0172..276e7ce952a 100644 --- a/include/Gaffer/ValuePlug.inl +++ b/include/Gaffer/ValuePlug.inl @@ -40,15 +40,12 @@ namespace Gaffer { template -boost::intrusive_ptr ValuePlug::getObjectValue( const IECore::MurmurHash *precomputedHash ) const +const T *ValuePlug::getObjectValue( IECore::ConstObjectPtr &owner, const IECore::MurmurHash *precomputedHash ) const { - IECore::ConstObjectPtr value = getValueInternal( precomputedHash ); + const IECore::Object *value = getValueInternal( owner, precomputedHash ); if( value && value->isInstanceOf( T::staticTypeId() ) ) { - // Steal the reference from `value`, to avoid incrementing and decrementing the refcount - // unnecessarily. - /// \todo Add a move-aware variant of `IECore::runTimeCast()` and use it instead. - return boost::intrusive_ptr( static_cast( value.detach() ), /* add_ref = */ false ); + return static_cast( value ); } throw IECore::Exception( fmt::format( @@ -57,4 +54,21 @@ boost::intrusive_ptr ValuePlug::getObjectValue( const IECore::MurmurHas ) ); } +template +boost::intrusive_ptr ValuePlug::getObjectValue( const IECore::MurmurHash *precomputedHash ) const +{ + IECore::ConstObjectPtr owner; + const T *value = getObjectValue( owner, precomputedHash ); + if( owner ) + { + // Avoid unnecessary reference count manipulations. + return boost::static_pointer_cast( std::move( owner ) ); + } + else + { + return value; + } + +} + } // namespace Gaffer diff --git a/python/GafferTest/ValuePlugTest.py b/python/GafferTest/ValuePlugTest.py index cd913ee67bc..0756bce3d93 100644 --- a/python/GafferTest/ValuePlugTest.py +++ b/python/GafferTest/ValuePlugTest.py @@ -700,6 +700,24 @@ def testStaticNumericValuePerformance( self ) : with GafferTest.TestRunner.PerformanceScope() : GafferTest.parallelGetValue( node["plug"], 10000000 ) + @GafferTest.TestRunner.PerformanceTestMethod() + def testStaticStringValuePerformance( self ) : + + node = Gaffer.Node() + node["plug"] = Gaffer.StringPlug() + + with GafferTest.TestRunner.PerformanceScope() : + GafferTest.parallelGetValue( node["plug"], 10000000 ) + + @GafferTest.TestRunner.PerformanceTestMethod() + def testStaticObjectValuePerformance( self ) : + + node = Gaffer.Node() + node["plug"] = Gaffer.ObjectPlug( defaultValue = IECore.IntVectorData() ) + + with GafferTest.TestRunner.PerformanceScope() : + GafferTest.parallelGetValue( node["plug"], 10000000 ) + def testIsSetToDefault( self ) : n1 = GafferTest.AddNode() diff --git a/src/Gaffer/StringPlug.cpp b/src/Gaffer/StringPlug.cpp index 7dc947cda3b..9e2adcf9049 100644 --- a/src/Gaffer/StringPlug.cpp +++ b/src/Gaffer/StringPlug.cpp @@ -108,7 +108,8 @@ void StringPlug::setValue( const std::filesystem::path &value ) std::string StringPlug::getValue( const IECore::MurmurHash *precomputedHash ) const { - ConstStringDataPtr s = getObjectValue( precomputedHash ); + ConstObjectPtr owner; + const StringData *s = getObjectValue( owner, precomputedHash ); const bool performSubstitutions = m_substitutions && @@ -146,7 +147,8 @@ IECore::MurmurHash StringPlug::hash() const if( performSubstitutions ) { - ConstStringDataPtr s = getObjectValue(); + ConstObjectPtr owner; + const StringData *s = getObjectValue( owner ); if( IECore::StringAlgo::hasSubstitutions( s->readable() ) ) { IECore::MurmurHash result; diff --git a/src/Gaffer/ValuePlug.cpp b/src/Gaffer/ValuePlug.cpp index 3272c17c587..61691845374 100644 --- a/src/Gaffer/ValuePlug.cpp +++ b/src/Gaffer/ValuePlug.cpp @@ -602,7 +602,7 @@ class ValuePlug::ComputeProcess : public Process g_cache.clear(); } - static IECore::ConstObjectPtr value( const ValuePlug *plug, const IECore::MurmurHash *precomputedHash ) + static const IECore::Object *value( const ValuePlug *plug, IECore::ConstObjectPtr &owner, const IECore::MurmurHash *precomputedHash ) { const ValuePlug *p = sourcePlug( plug ); @@ -617,7 +617,7 @@ class ValuePlug::ComputeProcess : public Process // No input connection, and no means of computing // a value. There can only ever be a single value, // which is stored directly on the plug. - return p->m_staticValue; + return p->m_staticValue.get(); } } @@ -646,16 +646,16 @@ class ValuePlug::ComputeProcess : public Process if( processKey.cachePolicy == CachePolicy::Uncached ) { - return ComputeProcess( processKey ).m_result; + return ComputeProcess( processKey, owner ).m_result.get(); } else if( Process::forceMonitoring( threadState, plug, ValuePlug::ComputeProcess::staticType ) ) { - ComputeProcess process( processKey ); + ComputeProcess process( processKey, owner ); g_cache.setIfUncached( processKey, process.m_result, []( const IECore::ConstObjectPtr &v ) { return v->memoryUsage(); } ); - return process.m_result; + return process.m_result.get(); } else if( processKey.cachePolicy == CachePolicy::Legacy ) { @@ -668,9 +668,10 @@ class ValuePlug::ComputeProcess : public Process if( auto result = g_cache.getIfCached( processKey ) ) { // Move avoids unnecessary additional addRef/removeRef. - return std::move( *result ); + owner = std::move( *result ); + return owner.get(); } - ComputeProcess process( processKey ); + ComputeProcess process( processKey, owner ); // Store the value in the cache, but only if it isn't there // already. The check is useful because it's common for an // upstream compute triggered by us to have already done the @@ -686,11 +687,12 @@ class ValuePlug::ComputeProcess : public Process processKey, process.m_result, []( const IECore::ConstObjectPtr &v ) { return v->memoryUsage(); } ); - return process.m_result; + return process.m_result.get(); } else { - return g_cache.get( processKey, currentContext->canceller() ); + owner = g_cache.get( processKey, currentContext->canceller() ); + return owner.get(); } } @@ -715,8 +717,8 @@ class ValuePlug::ComputeProcess : public Process private : - ComputeProcess( const ComputeProcessKey &key ) - : Process( staticType, key.plug, key.destinationPlug ) + ComputeProcess( const ComputeProcessKey &key, IECore::ConstObjectPtr &result ) + : Process( staticType, key.plug, key.destinationPlug ), m_result( result ) { try { @@ -760,22 +762,19 @@ class ValuePlug::ComputeProcess : public Process { case CachePolicy::Standard : { - ComputeProcess process( key ); - result = process.m_result; + ComputeProcess process( key, result ); break; } case CachePolicy::TaskCollaboration : { - ComputeProcess process( key ); - result = process.m_result; + ComputeProcess process( key, result ); break; } case CachePolicy::TaskIsolation : { tbb::this_task_arena::isolate( [&result, &key] { - ComputeProcess process( key ); - result = process.m_result; + ComputeProcess process( key, result ); } ); break; @@ -796,7 +795,7 @@ class ValuePlug::ComputeProcess : public Process using Cache = IECorePreview::LRUCache; static Cache g_cache; - IECore::ConstObjectPtr m_result; + IECore::ConstObjectPtr &m_result; }; @@ -1141,9 +1140,16 @@ const IECore::Object *ValuePlug::defaultObjectValue() const return m_defaultValue.get(); } +const IECore::Object *ValuePlug::getValueInternal( IECore::ConstObjectPtr &owner, const IECore::MurmurHash *precomputedHash ) const +{ + return ComputeProcess::value( this, owner, precomputedHash ); +} + IECore::ConstObjectPtr ValuePlug::getValueInternal( const IECore::MurmurHash *precomputedHash ) const { - return ComputeProcess::value( this, precomputedHash ); + IECore::ConstObjectPtr owner; + const IECore::Object *result = getValueInternal( owner, precomputedHash ); + return owner ? owner : result; } void ValuePlug::setObjectValue( IECore::ConstObjectPtr value ) diff --git a/src/GafferImage/AtomicFormatPlug.cpp b/src/GafferImage/AtomicFormatPlug.cpp index 3ef6693e1c8..58747b2d353 100644 --- a/src/GafferImage/AtomicFormatPlug.cpp +++ b/src/GafferImage/AtomicFormatPlug.cpp @@ -55,7 +55,8 @@ GAFFER_PLUG_DEFINE_TEMPLATE_TYPE( GafferImage::AtomicFormatPlug, AtomicFormatPlu template<> Format AtomicFormatPlug::getValue( const IECore::MurmurHash *precomputedHash ) const { - ConstFormatDataPtr d = getObjectValue( precomputedHash ); + IECore::ConstObjectPtr owner; + const FormatData *d = getObjectValue( owner, precomputedHash ); Format result = d->readable(); if( result.getDisplayWindow().isEmpty() && ( ( direction() == Plug::In && Process::current() ) || direction() == Plug::Out ) ) { diff --git a/src/GafferTestModule/ValuePlugTest.cpp b/src/GafferTestModule/ValuePlugTest.cpp index a8f1d2d3300..3e199703af1 100644 --- a/src/GafferTestModule/ValuePlugTest.cpp +++ b/src/GafferTestModule/ValuePlugTest.cpp @@ -40,6 +40,7 @@ #include "Gaffer/Context.h" #include "Gaffer/NumericPlug.h" +#include "Gaffer/StringPlug.h" #include "Gaffer/TypedObjectPlug.h" #include "Gaffer/ValuePlug.h" @@ -125,18 +126,22 @@ void GafferTestModule::bindValuePlugTest() { def( "repeatGetValue", &repeatGetValue ); def( "repeatGetValue", &repeatGetValue ); + def( "repeatGetValue", &repeatGetValue ); def( "repeatGetValue", &repeatGetValue ); def( "repeatGetValue", &repeatGetValue ); def( "repeatGetValue", &repeatGetValueWithVar ); def( "repeatGetValue", &repeatGetValueWithVar ); + def( "repeatGetValue", &repeatGetValueWithVar ); def( "repeatGetValue", &repeatGetValueWithVar ); def( "repeatGetValue", &repeatGetValueWithVar ); def( "parallelGetValue", ¶llelGetValue ); def( "parallelGetValue", ¶llelGetValue ); + def( "parallelGetValue", ¶llelGetValue ); def( "parallelGetValue", ¶llelGetValue ); def( "parallelGetValue", ¶llelGetValue ); def( "parallelGetValue", ¶llelGetValueWithVar ); def( "parallelGetValue", ¶llelGetValueWithVar ); + def( "parallelGetValue", ¶llelGetValueWithVar ); def( "parallelGetValue", ¶llelGetValueWithVar ); def( "parallelGetValue", ¶llelGetValueWithVar ); }