Fix a memory leak bug in ShadowDepthWrapper

[nonlinearthink]

this._subMeshToDepthWrapper.mm.delete(params.subMesh);

This code trigger a depth effect recreation but not release the previous depth effect in engine._compiledEffects, which cause a memory leak

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[nonlinearthink]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

I'm not contributor, can't add a labeI.

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[bjsplat]

Snapshot stored with reference name:
refs/pull/14406/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14406/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14406/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/14406/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/14406/merge
https://gui.babylonjs.com/?snapshot=refs/pull/14406/merge
https://nme.babylonjs.com/?snapshot=refs/pull/14406/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/14406/merge#BCU1XR#0

[Popov72]

This is by design, the _compiledEffects object is a cache and we don't clean it out during the course of a program, the idea being that we'll be able to reuse effects already compiled: there's normally a limited set of combinations for shader code, so the size of this object shouldn't increase indefinitely.

What's not normal is that you keep creating new effects in your program. I see some kind of GUID in your screenshot in the list of effect defines: where does it come from? I think that's the problem, it makes the system create a new effect for each GUID, even if all the other defines are the same.

[sebavan]

agree that those guids might be the root cause of the cache miss here. I ll close the current PR for know to prevent accidental merge and lets try to find how you can end up with them.

[nonlinearthink]

This is by design, the _compiledEffects object is a cache and we don't clean it out during the course of a program, the idea being that we'll be able to reuse effects already compiled: there's normally a limited set of combinations for shader code, so the size of this object shouldn't increase indefinitely.

What's not normal is that you keep creating new effects in your program. I see some kind of GUID in your screenshot in the list of effect defines: where does it come from? I think that's the problem, it makes the system create a new effect for each GUID, even if all the other defines are the same.

i think it happened if you change defines for a material, and this material has a ShadowDepthWrapper, it will trigger onEffectCreatedObservable in ShadowDepthWrapper, delete _subMeshToDepthWrapper, then a new depth effect will be created with a new GUID.

[Popov72]

Indeed, the shadow depth wrapper uses a guid to identify shaders, it's not a define as I originally thought. However, because of this, the shader can't really be reused later, so I agree that we can get rid of the effects when we need to recreate them for some reason.

I'm reopening the PR, but before we can approve it, you should make a separate function to factorize the code and avoid having the same block of code twice.

[Popov72]

You should remove this call, as effect.dispose() already does it.

[nonlinearthink]

You should remove this call, as effect.dispose() already does it.

it does, but there is still a big shader code string in memory, i don't kown why, so i try to call dispose() explicitly, then it works

[Popov72]

Are you sure the problem is coming from this? I don't see why calling first engine._releaseEffect would change anything... Are you able to provide a repro of the problem?

[nonlinearthink]

Are you sure the problem is coming from this? I don't see why calling first engine._releaseEffect would change anything... Are you able to provide a repro of the problem?

ok, i will try to build a playground tomorrow

[nonlinearthink]

https://playground.babylonjs.com/#C2PIGL

i just change albedoTexure per 500ms to trigger material defines change, see memoryLeakMaterial and memoryLeakTexture, my company's project is more complex

[Popov72]

I tested your PR by commenting the engine._releaseEffect(effect); line, and Object.keys(engine._compiledEffects).length remains constant (=9) during the execution of the PG (?)

Also, you should investigate why we get 12 times this warning in the console log:

[.WebGL-0000054400A15B00] GL_INVALID_VALUE: Program object expected.

We don't have them without the PR.

[nonlinearthink]

indeed, no need engine._releaseEffect(effect);, but i have no idea about this warning

[Popov72]

The fix for the warnings is a little involved (that's because the old effects should not be removed right away because of parallel shader compilation). I'm going to close this PR and will reopen a new one with your code and the fix for the warnings.

[Popov72]

In fact the problem was simpler, see line 126 in the other PR (#14449).