Merge pull request GafferHQ#5520 from johnhaddon/valuePlugGetValueOpt…

…imisation

ValuePlug : Avoid unnecessary reference counting

include/Gaffer/NumericPlug.inl

@@ -42,7 +42,8 @@ namespace Gaffer
 template<typename T>
 inline T NumericPlug<T>::getValue( const IECore::MurmurHash *precomputedHash ) const
 {
-	return getObjectValue<DataType>( precomputedHash )->readable();
+	IECore::ConstObjectPtr owner;
+	return getObjectValue<DataType>( owner, precomputedHash )->readable();
 }
 
 } // namespace Gaffer

include/Gaffer/TypedObjectPlug.inl

@@ -42,7 +42,17 @@ namespace Gaffer
 template<class T>
 inline typename TypedObjectPlug<T>::ConstValuePtr TypedObjectPlug<T>::getValue( const IECore::MurmurHash *precomputedHash ) const
 {
-	return getObjectValue<ValueType>( precomputedHash );
+	IECore::ConstObjectPtr owner;
+	const ValueType *value = getObjectValue<ValueType>( owner, precomputedHash );
+	if( owner )
+	{
+		// Avoid unnecessary reference count manipulations.
+		return boost::static_pointer_cast<const ValueType>( std::move( owner ) );
+	}
+	else
+	{
+		return ConstValuePtr( value );
+	}
 }
 
 } // namespace Gaffer

include/Gaffer/TypedPlug.inl

@@ -42,7 +42,8 @@ namespace Gaffer
 template<typename T>
 inline T TypedPlug<T>::getValue( const IECore::MurmurHash *precomputedHash ) const
 {
-	return getObjectValue<DataType>( precomputedHash )->readable();
+	IECore::ConstObjectPtr owner;
+	return getObjectValue<DataType>( owner, precomputedHash )->readable();
 }
 
 } // namespace Gaffer

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<typename T = IECore::Object>
+		const T *getObjectValue( IECore::ConstObjectPtr &owner, const IECore::MurmurHash *precomputedHash = nullptr ) const;
+		/// \deprecated
+		template<typename T = IECore::Object>
 		boost::intrusive_ptr<const T> 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();

include/Gaffer/ValuePlug.inl

@@ -40,15 +40,12 @@ namespace Gaffer
 {
 
 template<typename T>
-boost::intrusive_ptr<const T> 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<const T>( static_cast<const T *>( value.detach() ), /* add_ref = */ false );
+		return static_cast<const T *>( value );
 	}
 
 	throw IECore::Exception( fmt::format(
@@ -57,4 +54,21 @@ boost::intrusive_ptr<const T> ValuePlug::getObjectValue( const IECore::MurmurHas
 	) );
 }
 
+template<typename T>
+boost::intrusive_ptr<const T> ValuePlug::getObjectValue( const IECore::MurmurHash *precomputedHash ) const
+{
+	IECore::ConstObjectPtr owner;
+	const T *value = getObjectValue<T>( owner, precomputedHash );
+	if( owner )
+	{
+		// Avoid unnecessary reference count manipulations.
+		return boost::static_pointer_cast<const T>( std::move( owner ) );
+	}
+	else
+	{
+		return value;
+	}
+
+}
+
 } // namespace Gaffer

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()

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<StringData>( precomputedHash );
+	ConstObjectPtr owner;
+	const StringData *s = getObjectValue<StringData>( owner, precomputedHash );
 
 	const bool performSubstitutions =
 		m_substitutions &&
@@ -146,7 +147,8 @@ IECore::MurmurHash StringPlug::hash() const
 
 	if( performSubstitutions )
 	{
-		ConstStringDataPtr s = getObjectValue<StringData>();
+		ConstObjectPtr owner;
+		const StringData *s = getObjectValue<StringData>( owner );
 		if( IECore::StringAlgo::hasSubstitutions( s->readable() ) )
 		{
 			IECore::MurmurHash result;

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<ComputeProcess>(
+				owner = acquireCollaborativeResult<ComputeProcess>(
 					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 )

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<FormatData>( precomputedHash );
+	IECore::ConstObjectPtr owner;
+	const FormatData *d = getObjectValue<FormatData>( owner, precomputedHash );
 	Format result = d->readable();
 	if( result.getDisplayWindow().isEmpty() && ( ( direction() == Plug::In && Process::current() ) || direction() == Plug::Out ) )
 	{

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<IntPlug> );
 	def( "repeatGetValue", &repeatGetValue<FloatPlug> );
+	def( "repeatGetValue", &repeatGetValue<StringPlug> );
 	def( "repeatGetValue", &repeatGetValue<ObjectPlug> );
 	def( "repeatGetValue", &repeatGetValue<PathMatcherDataPlug> );
 	def( "repeatGetValue", &repeatGetValueWithVar<IntPlug> );
 	def( "repeatGetValue", &repeatGetValueWithVar<FloatPlug> );
+	def( "repeatGetValue", &repeatGetValueWithVar<StringPlug> );
 	def( "repeatGetValue", &repeatGetValueWithVar<ObjectPlug> );
 	def( "repeatGetValue", &repeatGetValueWithVar<PathMatcherDataPlug> );
 	def( "parallelGetValue", &parallelGetValue<IntPlug> );
 	def( "parallelGetValue", &parallelGetValue<FloatPlug> );
+	def( "parallelGetValue", &parallelGetValue<StringPlug> );
 	def( "parallelGetValue", &parallelGetValue<ObjectPlug> );
 	def( "parallelGetValue", &parallelGetValue<PathMatcherDataPlug> );
 	def( "parallelGetValue", &parallelGetValueWithVar<IntPlug> );
 	def( "parallelGetValue", &parallelGetValueWithVar<FloatPlug> );
+	def( "parallelGetValue", &parallelGetValueWithVar<StringPlug> );
 	def( "parallelGetValue", &parallelGetValueWithVar<ObjectPlug> );
 	def( "parallelGetValue", &parallelGetValueWithVar<PathMatcherDataPlug> );
 }