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 706ea41f349..c59be00535a 100644 --- a/include/Gaffer/ValuePlug.h +++ b/include/Gaffer/ValuePlug.h @@ -238,10 +238,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. @@ -250,6 +250,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 @@ -267,6 +270,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 95b245c9900..8b208e0b0bf 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 ad2087c0693..2393ccc23b2 100644 --- a/src/Gaffer/ValuePlug.cpp +++ b/src/Gaffer/ValuePlug.cpp @@ -508,7 +508,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 ); @@ -523,7 +523,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(); } } @@ -551,7 +551,8 @@ class ValuePlug::ComputeProcess : public Process if( cachePolicy == CachePolicy::Uncached ) { - return ComputeProcess( p, plug, computeNode ).run(); + owner = ComputeProcess( p, plug, computeNode ).run(); + return owner.get(); } const ThreadState &threadState = ThreadState::current(); @@ -573,7 +574,8 @@ class ValuePlug::ComputeProcess : public Process if( auto result = g_cache.getIfCached( hash ) ) { // Move avoids unnecessary additional addRef/removeRef. - return std::move( *result ); + owner = std::move( *result ); + return owner.get(); } } @@ -587,7 +589,7 @@ class ValuePlug::ComputeProcess : public Process // lightweight enough and unlikely enough to be shared that in // the worst case it's OK to do it redundantly on a few threads // before it gets cached. - IECore::ConstObjectPtr result = ComputeProcess( p, plug, computeNode ).run(); + owner = ComputeProcess( p, plug, computeNode ).run(); // 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 work, and calling @@ -598,14 +600,15 @@ class ValuePlug::ComputeProcess : public Process // upstream node will already have computed the same result) and the // attribute data itself consists of many small objects for which // computing memory usage is slow. - g_cache.setIfUncached( hash, result, cacheCostFunction ); - return result; + g_cache.setIfUncached( hash, owner, cacheCostFunction ); + return owner.get(); } else { - return acquireCollaborativeResult( + owner = acquireCollaborativeResult( hash, p, plug, computeNode ); + return owner.get(); } } @@ -664,7 +667,9 @@ class ValuePlug::ComputeProcess : public Process { throw IECore::Exception( "Compute did not set plug value." ); } - return m_result; + // Move to avoid unnecessary reference count increment/decrement - we don't + // need `m_result` any more. + return std::move( m_result ); } catch( ... ) { @@ -1030,9 +1035,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 ); }