WebGPU: Fix Renderer not being garbage collected

[shotamatsuda]

Description

This PR potentially fixes an issue that Renderer and all associated memory are not garbage collected (I say "potentially" because there may be other leak sources).

The geometry used in QuadMesh is instantiated in a global variable, and Geometries adds a dispose event listener that captures this in a closure and is never removed because the geometry is never disposed. Geometries holds a reference to Attributes, which holds a reference to a Backend, which holds a reference to Renderer, thus the GC cannot collect the entire renderer-related CPU memory.

I initially attempted to move QuadGeometry inside QuadMesh, but many QuadMesh instances are also instantiated in global variables. If we keep using it that way, we need to address it in Geometries.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

	Before	After	Diff
WebGL	338.24 79.14	338.24 79.14	+0 B +0 B
WebGPU	579.98 160.23	580.27 160.28	+289 B +56 B
WebGPU Nodes	578.59 159.99	578.88 160.04	+289 B +56 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

	Before	After	Diff
WebGL	469.88 113.85	469.88 113.85	+0 B +0 B
WebGPU	649.69 175.9	649.98 175.93	+289 B +39 B
WebGPU Nodes	603.8 165.05	604.09 165.11	+289 B +54 B

[shotamatsuda]

I'm not sure whether we can early-exit for already initialized geometries.

[Mugen87]

I think this check is redundant. The component already checks with its own map if the geometry is knows in updateForRender().

[shotamatsuda]

Yes, it makes sense. I'll remove it.

[shotamatsuda]

If Geometry.id cannot be a reliable source of uniqueness, I will replace it with a Map.

[Mugen87]

Do you mind explaining why? BufferGeometry.id is readonly, it is not honored in copy operations and not serialized. I can't think of valid situation where we would end up with multiple equal ids. Importing the engine multiple times is not a valid use case, btw.

[shotamatsuda]

Because maps like wireframes use WeakMap instead of Map or an object with id as keys. I'm fine with id keys if a collision in id is an invalid situation and doesn't need to be handled.

[Mugen87]

Because maps like wireframes use WeakMap instead of Map

Good point. In this case, using Map would indeed be more consistent.

[Mugen87]

As an alternative, how about we do not register an dispose event listener for QuadGeometry in the first place? In initGeometry() you have access to renderObject so it should be possible to check for isQuadMesh.

The changes in Geometries are a bit hard to follow, tbh. If we just have to make sure Geometries can be GC collected, ignoring the dispose handler seems the easiest solution.

If that is not acceptable, I would indeed suggest to stop using QuadMesh in global scope, put it in the scope of the component as a member, add QuadMesh.dispose() and call it in the components dispose() method.

[shotamatsuda]

After thinking a bit, I think neither adding an exception for QuadGeometry nor stopping the use of QuadMesh in the global scope is an ideal solution. The ideal solution would be that whatever adds an event listener must remove it when disposed.

I've seen many third-party libraries that rely on geometries in the global scope, which reasonably assume that memory is held only for geometry that is not disposed, not for the entire renderer-related memory. Additionally, WebGLRenderer has no issue with this.

If you can elaborate on what's hard to follow, I'm happy to adjust and add comments.

[Mugen87]

WebGLRenderer has no issue with this.

I've tried to reproduce the approach from WebGLRenderer by just having a single dispose() and avoiding reference to this. Would this code fix the issue Mugen87@f542070?

[shotamatsuda]

@Mugen87 Thank you. I tested your code but it doesn't solve this. Your onDispose callback function captures attributes in the closure and attributes holds a reference to the renderer.

[Mugen87]

With ECMAScript 2021, we could use WeakRef which should fix this issue. When attributes is wrapped into a weak reference, GC should not be blocked anymore.

Maybe we could also do this const scope = new WeakRef( this ); and use scope instead of this inside onDispose().

[shotamatsuda]

Technically, yes, but I haven't traced all the references that onDispose captures, so there might be others. Also, in my experience, the need for WeakRef is a sign of a design flaw. Nonetheless, that's my subjective opinion.

What confuses me is that you are trying to fix this issue (or to minimize the leaked memory footprint) without removing event listeners that Geometries attached even though it has already been disposed, and you are even suggesting introducing the use of WeakRef. My solution in this PR is very simple: remember the functions to remove event listeners, and execute them when Geometries is disposed. I'm not convinced by not removing the event listeners, nor am I informed of what's hard to follow in the first place.

[Mugen87]

Sorry for not explaining my hesitation but the need for separate functions for removing the listeners feels like a bandage as well. I'm just not feeling well with it and not with applying it to potentially other modules. I know this is somewhat a subjective decision but I feel we need to look for other solutions.

[Mugen87]

Also, in my experience, the need for WeakRef is a sign of a design flaw.

Do you mind elaborate why? Is that also true for similar structures like WeakMap?

[cmhhelgeson]

Also, in my experience, the need for WeakRef is a sign of a design flaw.

Do you mind elaborate why? Is that also true for similar structures like WeakMap?

I would also like some elaboration on this, if only to possibly understand how WeakRef and WeakMap might be related and why WeakMap does not seem to work as intended when caching GPU resources.

MDN seems to strongly discourage WeakRef use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef#avoid_where_possible.

[shotamatsuda]

WeakMap only allows weak references as keys. All we can do with it is look up an object that is guaranteed to have a strong reference, whereas WeakRef allows us to access the referent but we don't know whether the object has been collected when accessed, or whether it will be collected at all.

The gist of my opinion is that WeakRef is less deterministic. The referent could be collected at any time, if it is collected at all, and it often lives much longer than we expect. In a design pattern that has dispose(), we expect objects to be disposed in a deterministic way. This might also apply to WeakMap. If we can remove entries from a map in a deterministic way, that is always good. It gives the GC better hints and reduces the chance that the GC needs to perform a major GC.

Another reason is that WeakRef and coupled FinalizationRegistry are advanced features that require careful thought. We can already see it in your code, where attributes is strongly referenced (captured) by a closure, so adding WeakRef to this alone doesn't prevent it from leaking. Even if we manage to make the closure capture only weak references, we might later end up adding additional arguments or codes, I doubt we would take the same level of care again.

In my experience, there is almost always a way that doesn't require weak references (not just in JS, if uses an advanced GC). In this case, remove event listeners.

As to your hesitation about having functions to remove listeners (which I think is a very common way to implement cleanup functions, though), the following code does basically the same:

// In constructor
this._geometryListeners = {}

// In initGeometry()
this._geometryListeners[geometry.id] = { geometry, onDispose }

// In dispose()
for (const key in this._geometryListeners) {
  const { geometry, onDispose } = this._geometryListeners[key]
  geometry.removeEventListener('dispose', onDispose)
}

[Makio64]

I agree with @shotamatsuda about weakref / weakmap making the code less deterministic and more prompt to introduce bugs and memory leaks, typically what I recently encountered on 2 projects (world tiles system & massive images gallery )

In both project there are textures reference problems on heavy load/unload link to the dispose and the call to ref which doesnt exist suddenly ( #31747 ) .

When i isolated the problem on simple example it disapear so its probably linked to the gc underpressure deciding to remove refs in the middle of a render call.. a case not taken in account and due to the usage of weakmap. This is one potential bug in many other weakmap can introduce.

[shotamatsuda]

As an alternative to (or refinement of) the approach in this PR, we could introduce a reusable utility class that removes attached listeners on disposal, because I've seen several internal event listeners that are not removed.
This way somewhat forces the caller to remove them because DisposeListeners has a dispose() function and makes it easier to notice it must be disposed.

class DisposeListeners {
  constructor() {
    this._map = new Map()
  }

  attach(object, callback) {
    if (!this._map.has(object)) {
      const onDispose = () => {
        object.removeEventListener('dispose', onDispose)
        this._map.delete(object)
        callback()
      }
      object.addEventListener('dispose', onDispose)
      this._map.set(object, onDispose)
    }
  }

  remove(object) {
    if (this._map.has(object)) {
      object.removeEventListener('dispose', this._map.get(object))
      this._map.delete(object)
    }
  }

  dispose() {
    for (const [object, callback] of this._map.entries()) {
      object.removeEventListener('dispose', callback)
    }
    this._map.clear()
  }
}

Usage:

class Geometries {
  constructor() {
    this._disposeListeners = new DisposeListeners()
  }

  initGeometry(geometry) {
    this._disposeListeners.attach(geometry, () => {
      // Do some cleanup.
    })
  }

  dispose() {
    this._disposeListeners.dispose()
  }
}

[Mugen87]

Do you mind adding below comment above this line?

// see #31798 why tracking separate remove listeners is required right now
// TODO: Re-evaluate how onDispose() is managed in this component

I'm okay with merging the PR so the GC is fixed. We can add a different solution at a later point.