Fix issue with O(N^2) performance issue for addPart#17928
Fix issue with O(N^2) performance issue for addPart#17928CedricGuillemet merged 26 commits intoBabylonJS:masterfrom
Conversation
eliemichel
left a comment
eliemichel
left a comment
There was a problem hiding this comment.
I think that it would be easier and less invasive to change from addPart() to addParts(), then make sure to allocate mergedSplatsData only once in there with all the parts in it. You then only have a single call to this.updateData and thus only one call to _makeSplat.
The main drawback of my proposal is if you do not know ahead of time the full list of parts that you want to load and end up addParts() with a single part each time then you gain nothing and your approach is probably better.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17928/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
- Change parts to always store data to textures and rebuild textures on remove
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Reviewer - this PR has made changes to one or more package.json files. |
|
Thanks a lot for the fix. Moving as draft until @CedricGuillemet has a chance to review and decide if we can/should merge pre or post 9.0 |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17928/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
Smart Filters Editor is available to test at: |
|
WebGL2 visualization test reporter: |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
@spsDrop I think you may need to merge with master |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17928/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
|
@spsDrop It's working great with a little issue regarding visibility. Value is not taken into account: https://playground.babylonjs.com/?snapshot=refs/pull/17928/merge#BTS11N#0 |
|
OK fixed the bug with preserving transparency on remove. And integrated your PR in the new base class |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
Everytime we add a part we re-call _makeSplat for every part and every splat in those parts. This results in a worsening and worsening load time instead of an expected linear growth. Here is a performance flame graph showing the growing number of calls after adding 4 splats together.