USDLayerWriter performance improvements #6254
Conversation
| if( objectsMatch ) | ||
| { | ||
| objectsMatch = baseScene->objectPlug()->hash() == layerScene->objectPlug()->hash(); | ||
| canPrune = canPrune && objectsMatch; |
There was a problem hiding this comment.
Just an aesthetics thing, but I think I would find this a bit clearer if the first two assignments to canPrune were replaced with canPrune = canPrune && attributesMatch && objectsMatch in the canPrune branch below.
There was a problem hiding this comment.
I wasn't liking the aesthetics much here, so I thought I'd like this suggestion. But it means either introducing another if( canPrune ) conditional after doing that assignment, or potentially evaluating the transform unnecessarily if the assignment was false. So I've left as-is for now.
src/GafferUSD/USDLayerWriter.cpp
Outdated
| } | ||
| } | ||
|
|
||
| ScenePlug::ScenePath prefix; |
There was a problem hiding this comment.
I find the name prefix a bit odd here ... it didn't take too much investigation to conclude that everything here is correct: the name of the current location is the prefix that needs to be added to any downstream paths in order to make them relative to this path. But just seeing path.back() assigned to a var named "prefix" looks weird.
There was a problem hiding this comment.
I didn't like the name either, but I wasn't sure what else to call it. name didn't seem quite right since sometimes it is empty. In bf97464 I've renamed it to localPath and added a short comment - is that an improvement at all?
| taskGroupContext | ||
|
|
||
|
|
||
| ); |
There was a problem hiding this comment.
I think we've already talked about whether this could fit into a parallelGatherLocations style idiom, and you've concluded that not worrying about that is the best way to get this rolled out, which is totally reasonable. Might be worth a comment though - do you have any expectation about whether it's likely to improve performance to avoid the overhead of doing nested parallel_reduce? Or would it just be an aesthetic thing to make it fit in with other traversal?
There was a problem hiding this comment.
Added a comment in f99b188. I don't think there would be any performance difference in USDLayerWriter either way - a SceneAlgo version would still use nested parallel_reduce, just as parallelProcessLocations() uses nested parallel_for(). But there could be benefits to using this approach to building PathMatchers elsewhere.
1e7b222 to
f99b188
Compare
|
Thanks Daniel - I've replied to your latest comments, and also added a fixup for a problem I found in last-minute testing : 62d6ef5. |
|
LGTM |
Including documentation of a current failure case.
63bb9d0 to
e4535e3
Compare
This builds on #6232, improving USDLayerWriter performance by removing the identical parts of the scene before writing them to USD for diffing. I'm seeing a 50x speedup on a benchmark which changes shader assignment on a single location in the ALab scene, and close to a 5x speedup changing shader assignment on every location in the same scene.
Only the last two commits are unique to this PR - the others are borrowed from #6232.