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

WebGPURenderer: Per "texture set" bindGroup caching. #29845

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

aardgoose
Copy link
Contributor

Related issue: #29198, #27447

Alternative mechanism to avoid excessive bindGroup creation. Use a cache keyed by a value derived from the texture(s) used in a bindGroup to handle the situation where a material alternates between two or more textures. This caching only takes place for bindGroups that would be updated after the first pass.

BindGroups referencing defaultTextures or external textures are not cached.

Copy link

github-actions bot commented Nov 8, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.22
79.04
339.22
79.04
+0 B
+0 B
WebGPU 476.9
132.19
477.18
132.3
+282 B
+107 B
WebGPU Nodes 476.2
132.01
476.48
132.12
+282 B
+108 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.67
111.99
464.67
111.99
+0 B
+0 B
WebGPU 545.93
147.86
546.22
147.96
+282 B
+107 B
WebGPU Nodes 501.81
137.56
502.1
137.67
+282 B
+114 B


} else {

cacheIndex = cacheIndex * 10 + texture.id * 10 + texture.version;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of * 10?

Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some hash test that could use the NodeUtils hashArray() or hash()?

Copy link
Contributor Author

@aardgoose aardgoose Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?10 just to stop obvious collisions as a proof of concept. I am trying to avoid more string creation on a fairly hot path. A cheap hash would be better.

@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented Nov 8, 2024

Nice! I thought about something similar but the issue with this it that it's stackingbindGroup, Texture Views and Samplers.
Every resize event of a RenderTarget will dispose the RenderTarget and destroy all the FBO samplers associated resulting with this new logic that creates a new bindGroup, Texture Views and Samplers that never gets cleanup. On dev the issue is not there as the bindGroups never gets recreated since their textures aren't associated to them.

To complete your approach, a cleanup strategy for unused bindGroups which dispose of the associated texture views and samplers would likely resolve this. textureView = null and sampler = null should be enough to allow garbage collection in WebGPU as it doesn't provides an explicit .destroy() method for cleanup.

@aardgoose
Copy link
Contributor Author

Re garbage collection on resize. yes, it is an obvious issue. I just wanted to get a basic working approach to start with. After a bind group has been created, the only way to destroy it is to drop references to it AFAICS, so maybe we need a two part cacheId so that the version changes would cause a cached bindGroup to be overwritten? It is something I considered earlier.

@Mugen87 Mugen87 added this to the r171 milestone Nov 8, 2024
@RenaudRohlinger
Copy link
Collaborator

This new two part cacheId is an awesome solution and fixes the issue. Great PR!

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 11, 2024

@aardgoose Is this PR ready for review?

@aardgoose aardgoose marked this pull request as ready for review November 11, 2024 18:40
@Mugen87 Mugen87 changed the title WebGPURenderer: per "texture set" bindGroup caching WebGPURenderer: Per "texture set" bindGroup caching. Nov 12, 2024
@Mugen87 Mugen87 merged commit bb7f17a into mrdoob:dev Nov 13, 2024
12 checks passed
@aardgoose aardgoose deleted the bg-cache branch November 15, 2024 11:56
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.

3 participants