Skip to content

Conversation

@cmhhelgeson
Copy link
Contributor

@cmhhelgeson cmhhelgeson commented Nov 12, 2025

Related issue: #31682

Description

Access ids of previous used layout entries based on stringified result of bindingGPU, then use overall layout key to reuse existing bind group layouts.

Results

WebGPU Materials Toon goes from 440 Bind Group Layouts to 4

image image

WebGPU GLTF Loader Transmission goes from 16 to 7

image image

@github-actions
Copy link

github-actions bot commented Nov 12, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 356.24
86.48
356.24
86.48
+0 B
+0 B
WebGPU 615.49
172.75
616.33
172.94
+841 B
+185 B
WebGPU Nodes 614.1
172.5
614.94
172.68
+841 B
+183 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 488.04
121.25
488.04
121.25
+0 B
+0 B
WebGPU 684.96
188.02
685.8
188.21
+841 B
+190 B
WebGPU Nodes 626.68
171.22
627.52
171.41
+841 B
+187 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 12, 2025

TBH, I have the feeling the issue is fixed at the wrong place. There is already code in place that should ensure unique bind group layouts.

let bindLayoutGPU = bindGroupLayoutCache.get( bindGroup.bindingsReference );
if ( bindLayoutGPU === undefined ) {
bindLayoutGPU = this.createBindingsLayout( bindGroup );
bindGroupLayoutCache.set( bindGroup.bindingsReference, bindLayoutGPU );
}

This block is not correct. Instead of directly using the array bindingsReference, it would be better to compute a key based on the entries and then us it to query the layout. For a layout, what matters is the structure of the bindings, not the contents itself. Would you mind giving this a try? Something like:

const bindLayoutCacheKey = getBindLayoutCacheKey( bindGroup.bindingsReference );
let bindLayoutGPU = bindGroupLayoutCache.get( bindLayoutCacheKey ); 
  
 if ( bindLayoutGPU === undefined ) { 
  
 	bindLayoutGPU = this.createBindingsLayout( bindGroup ); 
 	bindGroupLayoutCache.set( bindLayoutCacheKey, bindLayoutGPU ); 
  
 } 

@cmhhelgeson
Copy link
Contributor Author

cmhhelgeson commented Nov 12, 2025

This block is not correct. Instead of directly using the array bindingsReference, it would be better to compute a key based on the entries and then us it to query the layout. For a layout, what matters is the structure of the bindings, not the contents itself. Would you mind giving this a try? Something like:

So you would prefer that I create a key from the full entries object rather than creating keys for each entry resource than collecting them?

I understand why this structure would be preferable, but the reason I create keys as the entries are created is that creating a key from the bindingsReference would essentially require us to iterate through each binding's properties to construct the hash for the overall layout entry. Then, if the hash is incorrect, we'd have to iterate through each binding again to actually generate the entries.

Could we just append to one key (rather than creating keys for buffer, texture, storageTexture, etc) over time as the bindGroupLayoutEntries are created? Then, if the key generated from the entries doesn't correspond to an entry in the map, we can execute the device.createBindGroupLayout call.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 12, 2025

TBH, I found the PR somewhat hard to follow. The code from #32249 (comment) should be easier to read even if that means to iterate through the array twice. This should no be an performance issue since this array will never be large.

@cmhhelgeson cmhhelgeson marked this pull request as draft November 13, 2025 03:16
@cmhhelgeson cmhhelgeson marked this pull request as ready for review November 13, 2025 07:49
@cmhhelgeson cmhhelgeson marked this pull request as draft November 13, 2025 23:21
@cmhhelgeson cmhhelgeson marked this pull request as ready for review November 14, 2025 17:24
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.

4 participants