From 4c1745ecf35878f797a2981e23243b8b1602ba14 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 21 Sep 2023 09:36:56 +0100 Subject: [PATCH 1/2] Plug Tests : Test `_precomputedHash` more robustly Our hash caching is now so aggressive that even if the precomputed hash was ignored, we'd still be pulling the hash out of the cache. So we now clear the cache to be sure that if the precomputed hash was ignored, that would trigger another call to `ComputeNode::hash()`. --- python/GafferTest/NumericPlugTest.py | 13 +++++-------- python/GafferTest/TypedPlugTest.py | 13 +++++-------- python/GafferTest/ValuePlugTest.py | 16 ++++++---------- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/python/GafferTest/NumericPlugTest.py b/python/GafferTest/NumericPlugTest.py index 92ab1a28992..3d092810732 100644 --- a/python/GafferTest/NumericPlugTest.py +++ b/python/GafferTest/NumericPlugTest.py @@ -450,17 +450,14 @@ def testPrecomputedHash( self ) : self.assertEqual( n.numComputeCalls, 1 ) h = n["sum"].hash() - numHashCalls = n.numHashCalls - # Accept either 1 or 2 - it would be reasonable for the ValuePlug - # to have either cached the hash or not, but that's not what we're - # testing here. - self.assertTrue( numHashCalls == 1 or numHashCalls == 2 ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) - # What we care about is that calling getValue() with a precomputed hash - # definitely doesn't recompute the hash again. + # Calling `getValue()`` with a precomputed hash shouldn't recompute the + # hash again, even if it has been cleared from the cache. + Gaffer.ValuePlug.clearHashCache() self.assertEqual( n["sum"].getValue( _precomputedHash = h ), 30 ) - self.assertEqual( n.numHashCalls, numHashCalls ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) def testIsSetToDefault( self ) : diff --git a/python/GafferTest/TypedPlugTest.py b/python/GafferTest/TypedPlugTest.py index 6c48492492d..b192e17d1fe 100644 --- a/python/GafferTest/TypedPlugTest.py +++ b/python/GafferTest/TypedPlugTest.py @@ -208,17 +208,14 @@ def testPrecomputedHash( self ) : self.assertEqual( n.numComputeCalls, 1 ) h = n["out"].hash() - numHashCalls = n.numHashCalls - # Accept either 1 or 2 - it would be reasonable for the ValuePlug - # to have either cached the hash or not, but that's not what we're - # testing here. - self.assertTrue( numHashCalls == 1 or numHashCalls == 2 ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) - # What we care about is that calling getValue() with a precomputed hash - # definitely doesn't recompute the hash again. + # Calling `getValue()` with a precomputed hash shouldn't recompute the + # hash again, even if it has been cleared from the cache. + Gaffer.ValuePlug.clearHashCache() self.assertEqual( n["out"].getValue( _precomputedHash = h ), "hi" ) - self.assertEqual( n.numHashCalls, numHashCalls ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) def testBoolPlugStringConnections( self ) : diff --git a/python/GafferTest/ValuePlugTest.py b/python/GafferTest/ValuePlugTest.py index 8b208e0b0bf..b74f5717aea 100644 --- a/python/GafferTest/ValuePlugTest.py +++ b/python/GafferTest/ValuePlugTest.py @@ -286,22 +286,18 @@ def testPrecomputedHashOptimisation( self ) : self.assertEqual( a1, IECore.StringData( "a" ) ) self.assertEqual( n.numHashCalls, 1 ) - # We apply some leeway in our test for how many hash calls are - # made - a good ValuePlug implementation will probably avoid - # unecessary repeated calls in most cases, but it's not - # what this unit test is about. a2 = n["out"].getValue( _copy = False ) self.assertTrue( a2.isSame( a1 ) ) - self.assertTrue( n.numHashCalls == 1 or n.numHashCalls == 2 ) + self.assertEqual( n.numHashCalls, 1 ) h = n["out"].hash() - self.assertTrue( n.numHashCalls >= 1 and n.numHashCalls <= 3 ) - numHashCalls = n.numHashCalls + self.assertEqual( n.numHashCalls, 1 ) - # What we care about is that calling getValue() with a precomputed hash - # definitely doesn't recompute the hash again. + # Calling `getValue()` with a precomputed hash shouldn't recompute the + # hash again, even if it has been cleared from the cache. + Gaffer.ValuePlug.clearHashCache() a3 = n["out"].getValue( _copy = False, _precomputedHash = h ) - self.assertEqual( n.numHashCalls, numHashCalls ) + self.assertEqual( n.numHashCalls, 1 ) self.assertTrue( a3.isSame( a1 ) ) def testSerialisationOfChildValues( self ) : From bdf288f649b00caf93ab28bdb61ff81a484afc51 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 2 Nov 2023 09:43:45 +0000 Subject: [PATCH 2/2] ValuePlug : Stop `getValueInternal()` ignoring `precomputedHash` --- src/Gaffer/ValuePlug.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Gaffer/ValuePlug.cpp b/src/Gaffer/ValuePlug.cpp index 2393ccc23b2..378c0cc68dd 100644 --- a/src/Gaffer/ValuePlug.cpp +++ b/src/Gaffer/ValuePlug.cpp @@ -567,7 +567,7 @@ class ValuePlug::ComputeProcess : public Process // > `StringPlug::hash()` account for additional processing (such as // > substitutions) performed in public `getValue()` methods _after_ // > calling `getValueInternal()`. - const IECore::MurmurHash hash = p->ValuePlug::hash(); + const IECore::MurmurHash hash = precomputedHash ? *precomputedHash : p->ValuePlug::hash(); if( !Process::forceMonitoring( threadState, plug, staticType ) ) {