RunTimeTypedWrapper : Avoid unnecessary GIL acquisitions

Changes

@@ -1,7 +1,16 @@
 10.7.x.x (relative to 10.7.0.0a10)
 ========
 
+Fixes
+-----
+
+- RunTimeTyped : Fixed unnecessary overhead of `runTimeCast()` and `isInstanceOf()` for subclasses implemented in Python. The GIL is now never needed when the cast is to a C++ type.
+
+Breaking Changes
+----------------
 
+- ClassParameter, ClassVectorParameter : Replaced statically allocated TypeIds with ids allocated dynamically by `IECore.registerRunTimeTyped()`. If the id is needed from C++, it can be queried at runtime with `RunTimeTyped::typeIdFromTypeName()`.
+- RunTimeTyped : Removed `typId` argument from `registerRunTimeTyped()` Python function. TypeIds are now always allocated dynamically for Python classes.
 
 10.7.0.0a10 (relative to 10.7.0.0a9)
 ===========

include/IECore/TypeIds.h

@@ -220,8 +220,8 @@ enum TypeId
 	FileSequenceParameterTypeId = 289,
 	FileSequenceVectorParameterTypeId = 290,
 	CompoundDataBaseTypeId = 311,
-	ClassParameterTypeId = 313,
-	ClassVectorParameterTypeId = 314,
+	ObsoleteClassParameterTypeId = 313, // Replaced by dynamically registered TypeId
+	ObsoleteClassVectorParameterTypeId = 314, // Replaced by dynamically registered TypeId
 	TransformationMatrixfParameterTypeId = 334,
 	TransformationMatrixdParameterTypeId = 335,
 	LineSegment3fDataTypeId = 344,
@@ -315,9 +315,11 @@ enum TypeId
 	FirstAtomsGafferTypeId = 128000,  // Used by AtomsGaffer
 	LastAtomsGafferTypeId = 128999,
 
-	// TypeIds dynamically allocated by registerRunTimeTyped (IECore Python)
-	FirstDynamicTypeId = 300000,
-	LastDynamicTypeId = 399999,
+	// TypeIds dynamically allocated for Python classes by
+	// `IECore.registerRunTimeTyped()`. Do not use for any
+	// other purpose.
+	FirstPythonTypeId = 300000,
+	LastPythonTypeId = 399999,
 
 	LastExtensionTypeId = 399999,
 	// Any TypeIds beyond this point can be considered safe for private internal use.

include/IECorePython/RunTimeTypedBinding.inl

@@ -137,9 +137,13 @@ const char *RunTimeTypedWrapper<T>::typeName() const
 template<typename T>
 bool RunTimeTypedWrapper<T>::isInstanceOf( IECore::TypeId typeId ) const
 {
-	if( T::isInstanceOf( typeId ) )
+	if( typeId < IECore::FirstPythonTypeId || typeId > IECore::LastPythonTypeId )
 	{
-		return true;
+		// TypeId belongs to a C++ class, so there is no need to check the
+		// result of Python override of `isInstanceOf()`. This is a crucial
+		// performance optimisation for `runTimeCast()` calls in C++, as it
+		// avoids taking the GIL unnecessarily.
+		return T::isInstanceOf( typeId );
 	}
 
 	if( this->isSubclassed() )
@@ -182,7 +186,7 @@ bool RunTimeTypedWrapper<T>::isInstanceOf( const char *typeName ) const
 				return boost::python::extract<bool>( res );
 			}
 		}
-		
+
 		catch( const boost::python::error_already_set & )
 		{
 			ExceptionAlgo::translatePythonException();

python/IECore/ClassParameter.py

@@ -103,6 +103,6 @@ def _parse( args, parameter ) :
 		parameter.setClass( args[0], int( args[1] ), args[2] )
 		del args[0:3]
 
-IECore.registerRunTimeTyped( ClassParameter, IECore.TypeId.ClassParameter )
+IECore.registerRunTimeTyped( ClassParameter )
 
 IECore.ParameterParser.registerType( ClassParameter.staticTypeId(), ClassParameter._parse, ClassParameter._serialise )

python/IECore/ClassVectorParameter.py

@@ -213,6 +213,6 @@ def _parse( args, parameter ) :
 
 		parameter.setClasses( list( zip( parameterNames, classNames, classVersions ) ) )
 
-IECore.registerRunTimeTyped( ClassVectorParameter, IECore.TypeId.ClassVectorParameter )
+IECore.registerRunTimeTyped( ClassVectorParameter )
 
 IECore.ParameterParser.registerType( ClassVectorParameter.staticTypeId(), ClassVectorParameter._parse, ClassVectorParameter._serialise )

python/IECore/registerRunTimeTyped.py

@@ -60,72 +60,52 @@ def __registerTypeId( typeId, typeName, baseTypeId ) :
 	# register the new type id
 	IECore.RunTimeTyped.registerType( typeId, typeName, baseTypeId )
 
-__nextDynamicRunTimeTypedId = None
+__nextTypeId = 300000 # Same as `TypeId::FirstPythonTypeId` defined in TypeIds.h
 
 ## This function adds the necessary function definitions to a python
 # class for it to properly implement the RunTimeTyped interface. It should
-# be called once for all python classes inheriting from RunTimeTyped. It also
-# calls registerTypeId() for you.
-# typId is optional and if not defined, this function will associate a dynamic Id
-#       in the range FirstDynamicTypeId and LastDynamicTypeId from TypeIds.h.
-#       It's necessary to specify type Id for Object derived class or anything that
-#       is serializable.
-# If typeName is not specified then the name of the class itself is used - you may wish
-# to provide an explicit typeName in order to prefix the name with a module name.
-def registerRunTimeTyped( typ, typId = None, typeName = None ) :
+# be called once for all python classes inheriting from RunTimeTyped.
+# If `typeName` is not specified then the name of the class itself is used -
+# you may wish to provide an explicit typeName in order to prefix the name
+# with a module name.
+## \todo It feels like this could probably be done automatically using a
+# custom metaclass for RunTimeTyped?
+def registerRunTimeTyped( typ, typeName = None ) :
 
 	if typeName is None :
 		typeName = typ.__name__
 
 	runTypedBaseClass = next( c for c in typ.__bases__ if issubclass( c, IECore.RunTimeTyped ) )
 
-	# constants below are the same as in TypeIds.h
-	FirstDynamicTypeId = 300000
-	LastDynamicTypeId = 399999
+	# As defined in TypeIds.h
+	LastPythonTypeId = 399999
 
-	# check if overwritting registration.
 	if not hasattr( IECore.TypeId, typeName ) :
 
-		if typId is None :
+		# First registration.
 
-			global __nextDynamicRunTimeTypedId
+		global __nextTypeId
+		if __nextTypeId > LastPythonTypeId :
+			raise Exception( "Too many dynamic RunTimeTyped registered classes! You must change TypeIds.h and rebuild Cortex." )
 
-			if __nextDynamicRunTimeTypedId is None :
-				__nextDynamicRunTimeTypedId = FirstDynamicTypeId
-			elif __nextDynamicRunTimeTypedId > LastDynamicTypeId:
-				raise Exception( "Too many dynamic RunTimeTyped registered classes! You must change TypeIds.h and rebuild Cortex." )
+		typeId = IECore.TypeId( __nextTypeId )
+		__nextTypeId += 1
 
-			typId = __nextDynamicRunTimeTypedId
+		__registerTypeId( typeId, typeName, IECore.TypeId( runTypedBaseClass.staticTypeId() ) )
 
-			__nextDynamicRunTimeTypedId += 1
+	else :
 
-		__registerTypeId( IECore.TypeId( typId ), typeName, IECore.TypeId( runTypedBaseClass.staticTypeId() ) )
+		# Re-registration - this can happen when reloading an Op for example.
 
-	else :
-		# check if the new type Id is compatible with the previously registered one.
-		prevTypId = getattr( IECore.TypeId, typeName )
-		if prevTypId in range( FirstDynamicTypeId, LastDynamicTypeId+1 ) :
-			if not typId is None :
-				raise Exception( "Trying to set a type ID for %s previously registered as a dynamic type Id!" % typeName )
-		else :
-			if typId is None :
-				raise Exception( "Trying to re-register type %s as dynamic type Id!" % typeName )
-			elif typId != prevTypId :
-				raise Exception( "Trying to re-register %s under different type Id: %s != %s" % ( typeName, str(typId), prevTypId ) )
-		# necessary when the typeid is defined in IECore/TypeIds.h and bound in TypeIdBinding.cpp, but then
-		# the class for that typeid is implemented in python (currently ClassParameter does this).
-		if IECore.RunTimeTyped.typeNameFromTypeId( prevTypId )=="" :
-			IECore.RunTimeTyped.registerType( prevTypId, typeName, IECore.TypeId( runTypedBaseClass.staticTypeId() ) )
-
-	# Retrieve the correct value from the enum
-	tId = getattr( IECore.TypeId, typeName )
+		typeId = getattr( IECore.TypeId, typeName )
+		assert( IECore.RunTimeTyped.typeNameFromTypeId( typeId ) != "" )
 
 	# add the typeId and typeName method overrides
-	typ.typeId = lambda x : tId
+	typ.typeId = lambda x : typeId
 	typ.typeName = lambda x: typeName
 
 	# add the staticTypeId, staticTypeName, baseTypeId, and baseTypeName overrides
-	typ.staticTypeId = staticmethod( lambda : tId )
+	typ.staticTypeId = staticmethod( lambda : typeId )
 	typ.staticTypeName = staticmethod( lambda : typeName )
 	typ.baseTypeId = staticmethod( lambda : runTypedBaseClass.staticTypeId() )
 	typ.baseTypeName = staticmethod( lambda : runTypedBaseClass.staticTypeName() )

src/IECorePython/TypeIdBinding.cpp

@@ -259,8 +259,6 @@ void bindTypeId()
 		.value( "FileSequenceParameter", FileSequenceParameterTypeId )
 		.value( "FileSequenceVectorParameter", FileSequenceVectorParameterTypeId )
 		.value( "CompoundDataBase", CompoundDataBaseTypeId )
-		.value( "ClassParameter", ClassParameterTypeId )
-		.value( "ClassVectorParameter", ClassVectorParameterTypeId )
 		.value( "TransformationMatrixfParameter", TransformationMatrixfParameterTypeId )
 		.value( "TransformationMatrixdParameter", TransformationMatrixdParameterTypeId )
 		.value( "LineSegment3fData", LineSegment3fDataTypeId )

test/IECore/ClassParameterTest.py

@@ -148,5 +148,17 @@ def testSerialiseAndParse( self ) :
 		self.assertEqual( c["a"].getNumericValue(), 10 )
 		self.assertEqual( c["b"].getNumericValue(), 20 )
 
+	def testTypeId( self ) :
+
+		parameter = IECore.ClassParameter(
+			"n",
+			"d",
+			"IECORE_OP_PATHS",
+			os.path.join( "maths", "multiply" ),
+			2
+		)
+
+		self.assertEqual( IECore.RunTimeTyped.typeIdFromTypeName( "ClassParameter" ), parameter.typeId() )
+
 if __name__ == "__main__" :
 	unittest.main()

test/IECore/ClassVectorParameterTest.py

@@ -514,6 +514,10 @@ def testUserData( self ) :
 
 		self.assertEqual( c["p0"].userData(), IECore.CompoundObject() )
 
+	def testTypeId( self ) :
+
+		parameter = IECore.ClassVectorParameter( "n", "d", "IECORE_OP_PATHS" )
+		self.assertEqual( IECore.RunTimeTyped.typeIdFromTypeName( "ClassVectorParameter" ), parameter.typeId() )
 
 if __name__ == "__main__" :
 	unittest.main()

test/IECore/RunTimeTyped.py

@@ -131,13 +131,6 @@ def testStaticTypeBindings( self ) :
 
 	def testRegisterRunTimeTyped( self ) :
 
-		# should raise because given type ID is different than the FileSequenceParameter type id
-		self.assertRaises( Exception, IECore.registerRunTimeTyped, IECore.FileSequenceParameter, 100009 )
-		# should raise because SequenceLsOp is registered with dynamic type id.
-		self.assertRaises( Exception, IECore.registerRunTimeTyped, IECore.SequenceLsOp, 100009 )
-		# should raise because FileSequenceParameter is registered with a non-dynamic type id
-		self.assertRaises( Exception, IECore.registerRunTimeTyped, IECore.FileSequenceParameter )
-
 		# should not raise because SequenceLsOp was already registered with a dynamic type id
 		IECore.registerRunTimeTyped( IECore.SequenceLsOp )