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

Added needsUpdate checks to WebGLRenderer.compile #17116

Closed
wants to merge 237 commits into from

Conversation

EliasHasle
Copy link
Contributor

@EliasHasle EliasHasle commented Jul 29, 2019

Before: initMaterial is called on every instance of every material:

  1. initMaterial invariably calls programCache.getProgramCode, which builds a large string from the shader.
  2. Also, the WebGLProperties object is queried with the material to obtain a copy of the previous code used for this material.
  3. If a code is found and it matches the one newly built, done.
  4. If, on the other hand, no code is found or they do not match, the code string is rebuilt once more after calling material.onBeforeCompile (NOTE: This could have been avoided if onBeforeCompile were null or undefined by default and the onBeforeCompile and getProgramCode calls were wrapped in a conditional),
  5. and then programCache.acquireProgram is called.
  6. acquireProgram invariably loops over the array of program codes, checking the new code against them, and breaking early only if it finds a match. The array is not ordered by usage frequency, so one can expect to loop over on average half the programs if there is a match, and (invariably) all the programs if there is no match. (Many string comparisons will obviously break early, or be "optimized away" by a good JS implementation.)

After: Shared materials will be initMaterialed only once. But they will all be initMaterialed, unless the user has overwritten the default needsUpdate=true.

Performance: Not measured on applications from the wild, but a stress test has been made (see comment further down) that demonstrates an improvement.

Side effects: Unknown. It puzzles me that initMaterial does not set needsUpdate=false itself. When is it useful to not set needsUpdate=false after initMaterial?

@EliasHasle
Copy link
Contributor Author

The only example that uses renderer.compile is https://threejs.org/examples/webgl_materials_compile.html. http://eliashasle.github.io/three.js/examples/webgl_materials_compile.html runs with both this and #17117. Doing a profiling of the page load in Google Chrome gives no clear difference, although the profiling runs for at least 3 seconds, which adds a lot of uncertainty sources. I think that differences may show more clearly in a tailored example where the compile takes a lot of time and there are really many objects and many that share material instances.

@EliasHasle
Copy link
Contributor Author

Stress test that shows improvement:
https://eliashasle.github.io/testing/renderer_compile_stress_status_quo.html
https://eliashasle.github.io/testing/renderer_compile_stress_patched.html

This is a rather extreme test with 600 000 meshes sharing a single RawShaderMaterial instance with extremely simple code, and the same simple geometry. It remains to be seen whether any real applications can benefit from this. That would be applications that precompile large scenes with heavy reuse of material instances. (Wouldn't have to be only a single material.)

@EliasHasle
Copy link
Contributor Author

@Usnul Do you have any take on this, considering the scale and relative homogeneity of your game world?

@Usnul
Copy link
Contributor

Usnul commented Aug 9, 2019

Hey, I just caught up with what's happening. I think it's a step in the right direction. Mybe it would be better to have a function called prepareMaterialForRendering or ensureMaterialReady or something along those lines. Because that's what it does under the hood, not strictly speaking "initializing" as you have pointed out yourself also.

With respect to string, I wrote a small static material cache a few weeks ago. It avoids the strings completely. There are still computationally intensive operations there, and it completely ignores onBeforeCompile. It uses a hash map, hashes are numeric in this case and so are much shorter (32bit) and are faster to compute, in my experience, than the entire source code string. Since it's a Map, there's still equality check in case of hash collisions - so you get best of both worlds: fast lookups and correct resolution.

Here's the topic with some musings on the subject: #16798

I haven't seen the checks themselves cause too much trouble for a while, mostly it's the shader compilation and texture image decoding that causes spikes in my case. I use quite a few different meshes in the game, they aren't prepared in any special way, just plain gltf files, so each has its own material and textures. Using the method I mentioned earlier I managed to reduce number of duplicate materials and textures, it doesn't really impact memory usage that much, but it does improve runtime performance considerably. In my case I got about 10% FPS increase on lower-end hardware.

mrdoob and others added 25 commits December 27, 2019 14:21
Fix d.ts signatures for setCrossOrigin and transformUv
Remove non-breaking space from code.
BufferGeometryUtils.js: Non-breaking space replaced with normal space
Deprecate Matrix*.applyToBufferAttribute() methods
Examples: added instancing_modified example
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 16, 2020

This PR needs an update because of #17968. Material.needsUpdate works now like all other update flags, it just increases an internal version counter. So it's not valid to set the flag to false anymore.

It puzzles me that initMaterial does not set needsUpdate=false itself. When is it useful to not set needsUpdate=false after initMaterial?

As you can see at the new code in WebGLRenderer, a call of initMaterial() does not necessarily increase the version counter:

if ( material.version === materialProperties.__version ) {
if ( materialProperties.program === undefined ) {
initMaterial( material, scene, object );
} else if ( material.fog && materialProperties.fog !== fog ) {
initMaterial( material, scene, object );
} else if ( materialProperties.environment !== environment ) {
initMaterial( material, scene, object );
} else if ( materialProperties.needsLights && ( materialProperties.lightsStateVersion !== lights.state.version ) ) {
initMaterial( material, scene, object );
} else if ( materialProperties.numClippingPlanes !== undefined &&
( materialProperties.numClippingPlanes !== _clipping.numPlanes ||
materialProperties.numIntersection !== _clipping.numIntersection ) ) {
initMaterial( material, scene, object );
} else if ( materialProperties.outputEncoding !== _this.outputEncoding ) {
initMaterial( material, scene, object );
}
} else {
initMaterial( material, scene, object );
materialProperties.__version = material.version;
}

In general, Material.needsUpdate should not be used internally. It's a flag intended for app-level code.

@EliasHasle
Copy link
Contributor Author

EliasHasle commented Jan 16, 2020

Thanks for your reply. I don't know exactly what changes would be needed. And I don't have time now. I doubt it is an important PR anyway, so far seeing results only on an extreme stress test. Should I just close the PR, or perhaps wait to see if anyone is interested in picking up on the "idea"?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2020

Should I just close the PR, or perhaps wait to see if anyone is interested in picking up on the "idea"?

I think we can close the PR until somebody else want's this feature. I mean WebGLRenderer.compile() is not used per frame and usually only called once per app.

@EliasHasle
Copy link
Contributor Author

EliasHasle commented Jan 17, 2020

The "feature" is/was to shave off a few unneeded updates during WebGLRenderer.compile(). With the refactoring, I am not sure how that would be achieved. I would have to look closer into the new code. Note that while extreme stress tests were needed to get a noticeable difference on my well-equipped gaming laptop running Chrome, it is quite possible that other platforms will experience problems on more realistic applications. But all right, I will close.

@EliasHasle EliasHasle closed this Jan 17, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 17, 2020

it is quite possible that other platforms will experience problems on more realistic applications.

Yes, that's true.

With the refactoring, I am not sure how that would be achieved. I would have to look closer into the new code.

I think it would be good if you can have a look. If it's not much code to add, we can still make the change.

@EliasHasle
Copy link
Contributor Author

EliasHasle commented Jan 17, 2020

EDIT: Nevermind, I used a local visited set instead. Not tested yet.

@EliasHasle EliasHasle reopened this Jan 17, 2020
Before: initMaterial is called on every instance of every material:
1. initMaterial invariably calls programCache.getProgramCode, which builds a large string from the shader.
2. Then the WebGLProperties object is queried with the material to obtain a copy of the previous code used for this material.
3. If a code is found and it matches the one newly built, done.
4. If, on the other hand, no code is found or they do not match, the code string is rebuilt once more after calling material.onBeforeCompile (NOTE: This could have been avoided if onBeforeCompile were null or undefined by default and the onBeforeCompile and getProgramCode calls were wrapped in a conditional),
5. and then programCache.acquireProgram is called.
6. acquireProgram invariably loops over the array of program codes, checking the new code against them, and breaking early only if it finds a match. The array is not ordered by usage frequency, so one can expect to loop over on average half the programs if there is a match, and (invariably) all the programs if there is no match. (Many string comparisons will obviously break early, or be "optimized away" by a good JS implementation.)

After: Shared materials will be `initMaterial`ed only once.

Performance: Not measured.

Side effects: Not tested.
@EliasHasle
Copy link
Contributor Author

Wait, this is just a mess. I will make a new PR from the correct base.

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.