Thin Instance vertex buffer memory leak (thinInstanceSetBuffer)

Bugs
1

Hello :slight_smile:

Played around with thin instances yesterday and saw that I was continuously getting more and more WebGL buffers in my context. Seems like there is a memory leak, see repro (comments and console logs):

Wasn’t able to track down the exact cause, and I’ve only tested the “specialized” matrix buffer. I’m fairly confident there is a memory leak here, but the underlying mechanics I can mostly speculate about, so don’t take my details here as a source of truth, it’s my guess of what’s going on.

Due to suspecting the issue being in 2 parts, I actually don’t think this is currently an issue for single-attribute buffers (as opposed to 4 in the matrix case) due to the fact that there seems to be an error in the reference counting causing it to “cancel out”. However, if this potential error in reference counting is fixed, the issue should show up for single-attribute buffers as well.

I hope the repro shows it well, but will write a summary here as well. I haven’t dug very deep so this is just my guess of what’s happening under the surface from what I could gather:

Allocation of new buffer:

  • Buffer is created with thinInstanceSetBuffer and reference count is not increased (guessing).
  • Inside ThinInstanceMesh::_thinInstanceCreateMatrixBuffer, setVerticesBuffer is called 4 times (one for each row). Each call increases the reference count to the underlying buffer, which is now at 4.

De-allocation of old buffer (happens at the same time):

  • Buffer is “disposed” by thinInstanceSetBuffer, decreasing the reference counter. I think this is a reference count error, as I suspect the original allocation did not increase the reference count. Either way, I believe we’re now left with 3 on the reference counter.
  • The aforementioned setVerticesBuffer do not seem to release their references, and the reference count is left at 3, and thus the underlying buffer is never disposed.

My thinking is that the flow should rather be that after thinInstanceSetBuffer we should have a reference count of 5 (or 2 in the single attribute cases), and the old/existing buffer should be fully disposed. The first reference being the allocation and storing inside _thinInstanceDataStorage (released when creating a new buffer), and the other 4 (or 1) being the vertex attribute references (which should also be released when replaced).

Cheers,
-Tore

2

cc @sebavan

4

Thanks a ton for the amazing repro, this is definitely a bug in Babylon. I ll address it ASAP as I want to be sure to fix it correctly :slight_smile:

5

I did a first pass at a fix Fix Thin Instances Memory leak by sebavan · Pull Request #16851 · BabylonJS/Babylon.js · GitHub if it creates issues I ll use a more targeted approach for the fix. Thanks a lot for the report.