-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
lighter getProgramCacheKey when using RawShaderMaterials #22650
Conversation
The gist of the problem: each new material instance gets run through this getProgramCacheKey function to see if three needs to build a program for that, or to connect a reference to an existing program. The getProgramCacheKey function is ‘ok’ for built in shaders, but it has a pretty huge allocation for custom shaders, by way of making the key out of concatenating the full strings of the fragment/vertex shaders. This wasn’t too much of an issue as long as the materials can be set up front, as it was a one time cost during load, but I'm noticing that with 3d-tiles based streaming, our current method of creating unique ShaderMaterial per tile, each new tile shown is a pretty large memory allocation, and for larger tilesets, where it’s possible to view thousands of tiles over time, this adds up to a healthy amount of GC pressure.
The gist of the problem: each new material instance gets run through this getProgramCacheKey function to see if three needs to build a program for that, or to connect a reference to an existing program. The getProgramCacheKey function is ‘ok’ for built in shaders, but it has a pretty huge allocation for custom shaders, by way of making the key out of concatenating the full strings of the fragment/vertex shaders. This wasn’t too much of an issue as long as the materials can be set up front, as it was a one time cost during load, but I'm noticing that with 3d-tiles based streaming, our current method of creating unique ShaderMaterial per tile, each new tile shown is a pretty large memory allocation, and for larger tilesets, where it’s possible to view thousands of tiles over time, this adds up to a healthy amount of GC pressure.
The gist of the problem: each new material instance gets run through this getProgramCacheKey function to see if three needs to build a program for that, or to connect a reference to an existing program. The getProgramCacheKey function is ‘ok’ for built in shaders, but it has a pretty huge allocation for custom shaders, by way of making the key out of concatenating the full strings of the fragment/vertex shaders. This wasn’t too much of an issue as long as the materials can be set up front, as it was a one time cost during load, but I'm noticing that with 3d-tiles based streaming, our current method of creating unique ShaderMaterial per tile, each new tile shown is a pretty large memory allocation, and for larger tilesets, where it’s possible to view thousands of tiles over time, this adds up to a healthy amount of GC pressure.
Very nice! Lets give this a try 👍 |
Thanks! |
Oh man, this is done with good intentions, but it's incorrect. There's a concept of hash collision. What you have created is something that assumes that hash equality is the same as entity equality. Which is just not the case. To put it simply - currently it's possible for 2 completely different shaders to have IDENTICAL key. Please revert this. The probability of this happening is very low, if your hash function is good, but very low is not good enough. For comparison, v3 UUID (that is the random variant) has 128 bits, and I have seen collisions of those in real life. Yet here we are dealing with 32 bits only. |
…oob#22650)" This reverts commit 20e2e54.
Indeed, this is a valid point. Hashes should not be used as UUIDs by definition. |
Related issue: #22530
Description
The gist of the problem: each new material instance gets run through this getProgramCacheKey function to see if three needs to build a program for that, or to connect a reference to an existing program.
The getProgramCacheKey function is ‘ok’ for built in shaders, but it has a pretty huge allocation for custom shaders, especially a relatively large ubershader, by way of making the key out of concatenating the full strings of the fragment/vertex shaders.
This wasn’t too much of an issue as long as the materials can be set up front, as it was a one time cost during load, but I'm noticing that with 3d-tiles based streaming, our current method of creating unique ShaderMaterial per tile, each new tile shown is a pretty large memory allocation, and for larger tilesets, where it’s possible to view thousands of tiles over time, this adds up to a healthy amount of GC pressure.
Hash function choice 1000% up for debate, this one was working fairly well, quickly, and with much lower allocation pressure showing in testing.
Before:
After: