Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instancer : More Robust Prototype Hashing #6272

Conversation

danieldresser-ie
Copy link
Contributor

Building on top of the recently merged hierarchyHash, and the previous fix that hasn't been merged yet #6258, this addresses the issue we identified where an unrelated change to the scene would force instancer capsules to be regenerated.

I'm not sure what the right strategy is for merging these ... it's close enough now that it might make sense to roll on ahead and get the relative prototype path thing working properly, and confirm that this work all works for where we want to get to. Maybe then we merge it as one big PR? Or maybe we come back and start merging these partial steps once we've confirmed they get us where we need to be?

For the moment though, the main thing I'd like to verify is that you're happy with the last commit here, which adds an extra plug for caching the new expensive hash. It's a bit of extra complexity, but it's necessary if we don't want a performance regression in the case of making changes to the input scene that don't affect the engine, while using heavy prototypes that are independent from the input scene.

@danieldresser-ie
Copy link
Contributor Author

These are the test results for the last 4 commits:

Instancer : Add tests before prototype hash rework
Baseline before this fix. Hash is very cheap, but the new test shows it sometimes unnecessarily changes.

testCacheExpensivePrototypeHash (GafferSceneTest.InstancerTest.InstancerTest) ... ok
    Times : 4.43e-05s, 5.25e-05s, 4.6e-05s
    Best  : 4.43e-05s
testPrototypeHashPerf (GafferSceneTest.InstancerTest.InstancerTest) ... ok
    Times : 0.0261s, 0.0201s, 0.0207s
    Best  : 0.0201s
testUnrelatedPrototypeChange (GafferSceneTest.InstancerTest.InstancerTest) ... FAIL

Instancer : More accurate prototype hash
Fixed issue, but both performance tests for prototype hashing got a lot slower

======================================================================
FAIL: testCacheExpensivePrototypeHash (GafferSceneTest.InstancerTest.InstancerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/danield/dev/gaffer-build/python/GafferTest/TestRunner.py", line 128, in wrapper
    args[0].assertLessEqual( min( timings ), min( previousTimings ) + self.__acceptableDifference )
AssertionError: 0.0690157413482666 not less than or equal to 0.010040769577026367

======================================================================
FAIL: testPrototypeHashPerf (GafferSceneTest.InstancerTest.InstancerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/danield/dev/gaffer-build/python/GafferTest/TestRunner.py", line 128, in wrapper
    args[0].assertLessEqual( min( timings ), min( previousTimings ) + self.__acceptableDifference )
AssertionError: 0.5068230628967285 not less than or equal to 0.029146919250488283

testUnrelatedPrototypeChange (GafferSceneTest.InstancerTest.InstancerTest) ... ok

FIX : Instancer : Parallelize
Parallelizing speeds up one test a lot. The new hash is still 0.1s more expensive than the old one, but the difference is probably acceptable for what were gaining. The case where the in plug is dirtied without affecting anything though is still noticeably slow, whereas previously it was basically zero cost.

======================================================================
FAIL: testCacheExpensivePrototypeHash (GafferSceneTest.InstancerTest.InstancerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/danield/dev/gaffer-build/python/GafferTest/TestRunner.py", line 128, in wrapper
    args[0].assertLessEqual( min( timings ), min( previousTimings ) + self.__acceptableDifference )
AssertionError: 0.07572293281555176 not less than or equal to 0.010040769577026367

======================================================================
FAIL: testPrototypeHashPerf (GafferSceneTest.InstancerTest.InstancerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/danield/dev/gaffer-build/python/GafferTest/TestRunner.py", line 128, in wrapper
    args[0].assertLessEqual( min( timings ), min( previousTimings ) + self.__acceptableDifference )
AssertionError: 0.12815546989440918 not less than or equal to 0.029146919250488283

testUnrelatedPrototypeChange (GafferSceneTest.InstancerTest.InstancerTest) ... ok

FIX : Instancer : Cache new prototype hash computation
This adds the new cache plug, fixing the case with the massive regression.

testCacheExpensivePrototypeHash (GafferSceneTest.InstancerTest.InstancerTest) ... ok
    Times : 4.89e-05s, 7.49e-05s, 5.13e-05s
    Best  : 4.89e-05s

======================================================================
FAIL: testPrototypeHashPerf (GafferSceneTest.InstancerTest.InstancerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/danield/dev/gaffer-build/python/GafferTest/TestRunner.py", line 128, in wrapper
    args[0].assertLessEqual( min( timings ), min( previousTimings ) + self.__acceptableDifference )
AssertionError: 0.1284182071685791 not less than or equal to 0.029146919250488283


testUnrelatedPrototypeChange (GafferSceneTest.InstancerTest.InstancerTest) ... ok

@johnhaddon
Copy link
Member

the main thing I'd like to verify is that you're happy with the last commit here, which adds an extra plug for caching the new expensive hash

I think using a separate plug for it is actually necessary, because we'll need to use the TaskCollaboration cache policy for this compute (since it's TBB based). Would be good to update the PR to include that.

@johnhaddon
Copy link
Member

Closing in favour of #6248.

@johnhaddon johnhaddon closed this Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants