perf: optimize RemoveUnusedMaterialProperties #1326
Conversation
This change improves the performance of RemoveUnusedMaterialProperties, by using native SerializedProperty calls to move items, rather than a C#-side copy function. This avoids a lot of marshalling and GC. This provides a performance improvement of 340ms -> 68ms on my main avatar.
This pass was obtaining the blendshape buffer for each vertex, then processing those buffers. However, in practice, most vertices share the same buffer; this pass ignored this, and redid the same work thousands of times as a result. This reduces processing time from ~370ms to ~22ms on my main avatar.
|
I will be making further changes; please hold off on merging for now. |
This demonstrates that there's a lack of unit tests for this codepath; I'll leave it in draft until I have a good way to test.
|
This change needs additional unit tests before it should be merged. |
| // This MoveArrayElement call effectively rotates the range of elements | ||
| // between from and to. As such, since we are iterating from the start, | ||
| // each prior rotation doesn't affect the "from" index of subsequent elements. | ||
| props.MoveArrayElement(from, to); |
There was a problem hiding this comment.
I did not know this function. thank you
There was a problem hiding this comment.
It's still more costly than ideal, but much better than before...
Test~/Basic/RemoveUnusedMaterialProperties/RemoveUnusedMaterialPropertiesTest.cs
Outdated
Show resolved
Hide resolved
| - _AUnusedTex0: | ||
| m_Texture: {fileID: 2800000, guid: 5ab62c479f88d4aada9d7ae22a8224d5, type: 3} | ||
| m_Scale: {x: 1, y: 1} | ||
| m_Offset: {x: 0, y: 0} | ||
| - _AUnusedTex1: | ||
| m_Texture: {fileID: 2800000, guid: 33780cc7041974dcea00d6a3c3de4a3e, type: 3} | ||
| m_Scale: {x: 1, y: 1} | ||
| m_Offset: {x: 0, y: 0} | ||
| - _AUnusedTex2: | ||
| m_Texture: {fileID: 2800000, guid: eed4f0bcec3d849eba0543fba412318d, type: 3} | ||
| m_Scale: {x: 1, y: 1} | ||
| m_Offset: {x: 0, y: 0} | ||
| - _MainTex: | ||
| m_Texture: {fileID: 2800000, guid: 8558587c82bb240ba949e06cc79f1f10, type: 3} | ||
| m_Scale: {x: 1, y: 1} | ||
| m_Offset: {x: 0, y: 0} | ||
| - _MainTex2nd: | ||
| m_Texture: {fileID: 2800000, guid: 576604d4986f24aaab960da95142db80, type: 3} | ||
| m_Scale: {x: 1, y: 1} | ||
| m_Offset: {x: 0, y: 0} |
There was a problem hiding this comment.
Should we intersperse these? I don't think this will trigger all codepaths in the new optimized code.
|
I have added tests for RemoveUnusedMaterialProperties and I confirmed both moving and removing pass will be checked with the unit test with debugger and coverage report. 
|
|
Good to know we have coverage; I still think we should assert on the main purpose of this code, which is to actually remove those properties... |
|
I think it's the most important to not break existing avatar so I implemented test for that. I'm considering how do we implement test for removing property. |
|
Thank you for performance improvements and checking tests. |
This change improves the performance of RemoveUnusedMaterialProperties,
by using native SerializedProperty calls to move items, rather than a
C#-side copy function. This avoids a lot of marshalling and GC.
This provides a performance improvement of 340ms -> 68ms on my main
avatar.