WebGLShaderCache poor performance

[jhurliman]

Describe the bug

The WebGLShaderCache uses full vertex/fragment shader source code as keys in a Map<string, WebGLShaderStage> and performs several lookups per frame, causing noticeably poor performance.

To Reproduce

1. Create a RawShaderMaterial subclass with a non-trivial vertex and/or fragment shader (a few hundred lines of GLSL)
2. Render 10k object instances using this material
3. Record a performance trace in Chrome dev tools
4. Observe the time spent in WebGLShaderCache#_getShaderStage()

Expected behavior

• Avoid double-lookups in the common path: instead of .has() followed by .get(), use a single .get() when possible
• Allow materials to specify their own vertexShaderKey and fragmentShaderKey strings that will be used instead of the full shader source code

I have made both of these changes in a patched version of three.js we use in Foxglove Studio (see https://github.com/foxglove/studio/blob/main/patches/three.patch). I'd be happy to turn this into a PR for three.js if this seems like the right approach.

Platform:

• Device: All
• OS: All
• Browser: All
• Three.js version: dev

[Mugen87]

I'd like to split things up and start with:

Avoid double-lookups in the common path: instead of .has() followed by .get(), use a single .get() when possible

Do you mind filing a PR with this change?

[jhurliman]

I'd like to split things up and start with:

Avoid double-lookups in the common path: instead of .has() followed by .get(), use a single .get() when possible

Do you mind filing a PR with this change?

https://github.com/mrdoob/three.js/pull/24457

[Mugen87]

Allow materials to specify their own vertexShaderKey and fragmentShaderKey strings that will be used instead of the full shader source code

I'm not a fan of exposing such deep engine internals to users. If the internal caching mechanism are not fast enough, we should find different ways to improve the performance but not by delegating key or ID computations to user level code.

Instead of:

const material = new ShaderMaterial( {
    uniforms: UniformsUtils.clone( shader.uniforms ),
    fragmentShader: shader.fragmentShader,
    vertexShader: shader.vertexShader
} );

one could think of:

const vertexShader = new WebGLShader( shader.vertexShader );
const fragmentShader = new WebGLShader( shader.fragmentShader );

const material = new ShaderMaterial( {
    uniforms: UniformsUtils.clone( shader.uniforms ),
    fragmentShader: vertexShader,
    vertexShader: fragmentShader
} );

Devs can then reuse instances of WebGLShader across different custom materials. This would make it easy to identify unique shaders since the renderer could simply use the instance of WebGLShader as a key. However, this would also be a noticeable API change and I'm not sure the respective performance benefit makes it worth.

[jhurliman]

Another option could be generating hashes of the shader sources and storing them as material properties that are used for lookups instead of the full source. I think this would be possible without an API change.

[jtbandes]

Would that require assuming equal hashes implies equal strings in order to get the performance benefit?

[gero3]

@jhurliman the creation of the hashes is slower than the full-text search on the map object. This was the implementation before this implementation of the shader key. This was changed in the following PR: #23022.

I would propose a somewhat more complex solution but with the advantage that it only deals with the full text of the shader when the shader text changes. If we dispatch events when the text of a shader changes to tell the renderer that it should update the internal Identifier of the shader text the next time the material gets used then it would only deal with the full text on that frame and not on other frames.

[Mugen87]

If we dispatch events when the text of a shader changes to tell the renderer that it should update the internal Identifier of the shader text the next time the material gets used then it would only deal with the full text on that frame and not on other frames.

Maybe we can make the material policy more strict and state that the vertex and fragment shader can't be changed after a shader material has been created. This would make the implementation much easier since a hash could be computed once during the creation time and that's it. If vertexShader, fragmentShader need to be updated, a new shader material has to be created.

We have similar strict policies in place for performance and simplicity reasons. E.g. the dimensions, format, and type of a texture can not be changed after its creation. It's also not possible to resize buffer attributes after their creation.

[gero3]

Maybe we can make the material policy more strict and state that the vertex and fragment shader can't be changed after a shader material has been created. This would make the implementation much easier since a hash could be computed once during the creation time and that's it. If vertexShader, fragmentShader need to be updated, a new shader material has to be created.

It would make it easy to detect when there are possibly new shader texts and it also simplifies the caching since materials only have 1 set of shader text possible. It would need to be tested in the wild to see how much influence that has on existing code. Since I'm not sure if changing shadertext is a common thing.

[Mugen87]

Since I'm not sure if changing shadertext is a common thing.

I don't think so. @mrdoob How do you feel about this?

[gero3]

@Mugen87

There are several examples with onBeforeCompile which set the vertex shader just before compilation, so it seems the paradigm of setting shaders after creation is something that has more impact than I first envisioned.

Examples that use onBeforeCompile to change the shader text (this is not a complete list)

• webgl_gpgpu_birds_gltf.html
• webgl_materials_modified.html

[Mugen87]

I've forgot onBeforeCompile() 🤦. In this case, the hash generation at creation time does not work.

[mrdoob]

But we could turn vertexShader and fragmentShader into getters/setters and set needsUpdate to true?

[jtbandes]

Hi all – any further thoughts on how to tackle this issue? We are still maintaining a patch at https://github.com/foxglove/studio/blob/main/patches/three.patch and it would be great to find a solution that everyone can use.