Merge pull request #5503 from johnhaddon/regressionFixes

Fix `ValuePlug::getValue()` performance regression

Changes.md

@@ -8,10 +8,12 @@ Improvements
 - Spreadsheet :
   - Popups for string cells and row names are now sized to fit their column.
   - Added "Triple" and "Quadruple" width options to the spreadsheet row name popup menu.
+- Node : Improved performance when casting Python-derived types to ComputeNode.
 
 Fixes
 -----
 
+- ValuePlug : Fixed performance regression (introduced in 1.3.1.0) getting values from plugs without an input connection. This could severely affect scene generation times in some cases.
 - NameSwitch : Fixed bug which prevented drag and drop reordering of rows with an input connection.
 - PythonEditor :
   - Fixed output for `print()` calls with multiple arguments, which was previously spread across multiple lines.

include/GafferBindings/NodeBinding.h

@@ -101,9 +101,10 @@ class NodeWrapper : public GraphComponentWrapper<T>
 				// the `Dispatcher::dispatch()` process.
 				typeId == (IECore::TypeId)Gaffer::ContextProcessorTypeId ||
 				typeId == (IECore::TypeId)Gaffer::SwitchTypeId ||
-				// ScriptNode, DependencyNode and EditScope also appear on
-				// performance critical code paths.
+				// ScriptNode, DependencyNode, ComputeNode and EditScope also
+				// appear on performance critical code paths.
 				typeId == (IECore::TypeId)Gaffer::ScriptNodeTypeId ||
+				typeId == (IECore::TypeId)Gaffer::ComputeNodeTypeId ||
 				typeId == (IECore::TypeId)Gaffer::DependencyNodeTypeId ||
 				typeId == (IECore::TypeId)Gaffer::EditScopeTypeId
 			)

python/GafferTest/BoxTest.py

@@ -1119,5 +1119,24 @@ def assertPassThrough( script ) :
 
 		assertPassThrough( s2 )
 
+	def testComputeNodeCastDoesntRequirePython( self ) :
+
+		class CastChecker( Gaffer.Box ) :
+
+			def __init__( self, name = "CastChecker" ) :
+
+				Gaffer.Box.__init__( self, name )
+				self["out"] = Gaffer.IntPlug( direction = Gaffer.Plug.Direction.Out )
+
+			def isInstanceOf( self, typeId ) :
+
+				raise Exception( "Cast to ComputeNode should not require Python" )
+
+		# The call to `dependsOnCompute()` will internally cast to `ComputeNode`
+		# in C++. We don't want that to require entry into Python because it is
+		# far too costly and the answer can be determined on the C++ side anyway.
+		node = CastChecker()
+		self.assertFalse( Gaffer.PlugAlgo.dependsOnCompute( node["out"] ) )
+
 if __name__ == "__main__":
 	unittest.main()

python/GafferTest/ValuePlugTest.py

@@ -691,6 +691,15 @@ def testContentionForOneItem( self ) :
 		with GafferTest.TestRunner.PerformanceScope() :
 			GafferTest.parallelGetValue( m["product"], 10000000 )
 
+	@GafferTest.TestRunner.PerformanceTestMethod()
+	def testStaticNumericValuePerformance( self ) :
+
+		node = Gaffer.Node()
+		node["plug"] = Gaffer.IntPlug()
+
+		with GafferTest.TestRunner.PerformanceScope() :
+			GafferTest.parallelGetValue( node["plug"], 10000000 )
+
 	def testIsSetToDefault( self ) :
 
 		n1 = GafferTest.AddNode()
@@ -1012,6 +1021,21 @@ def testNoCachePolicyForConversions( self ) :
 		n["in"].hash()
 		self.assertEqual( n["in"].getValue(), False )
 
+	def testOutputPlugWithConvertingInput( self ) :
+
+		for nodeType in ( Gaffer.Node, Gaffer.ComputeNode ) :
+			with self.subTest( nodeType = nodeType ) :
+
+				node = nodeType()
+				node["in"] = Gaffer.IntPlug()
+				node["out"] = Gaffer.FloatPlug( direction = Gaffer.Plug.Direction.Out )
+				node["out"].setInput( node["in"] )
+
+				for i in range( 0, 10 ) :
+
+					node["in"].setValue( i )
+					self.assertEqual( node["out"].getValue(), i )
+
 	def setUp( self ) :
 
 		GafferTest.TestCase.setUp( self )

src/Gaffer/ValuePlug.cpp

@@ -606,11 +606,13 @@ class ValuePlug::ComputeProcess : public Process
 		{
 			const ValuePlug *p = sourcePlug( plug );
 
-			const ComputeNode *computeNode = IECore::runTimeCast<const ComputeNode>( p->node() );
-
+			const ComputeNode *computeNode = nullptr;
 			if( !p->getInput() )
 			{
-				if( p->direction()==In || !computeNode )
+				// Note the assignment to `computeNode` for use later. It is important that this is short-circuited
+				// for input plugs, as the unnecessary `runTimeCast()` would add measurable overhead for our most
+				// common case.
+				if( p->direction()==In || !(computeNode = IECore::runTimeCast<const ComputeNode>( p->node() )) )
 				{
 					// No input connection, and no means of computing
 					// a value. There can only ever be a single value,
@@ -623,6 +625,8 @@ class ValuePlug::ComputeProcess : public Process
 			// one per context, computed via ComputeNode::compute(). Pull the value out of our cache, or compute
 			// it with a ComputeProcess.
 
+			assert( (plug->getInput() && !computeNode) || computeNode );
+
 			const ThreadState &threadState = ThreadState::current();
 			const Context *currentContext = threadState.context();