PrismarineJS / prismarine-web-client

Minecraft web client running in your browser
https://prismarinejs.github.io/prismarine-web-client/
MIT License
435 stars 122 forks source link

Memory leak in prismarine-web-client #333

Closed kf106 closed 1 year ago

kf106 commented 1 year ago

OS: Ubuntu 22.04 (probably others - will investigates later) Browser: Chrome 108.0.5359.124 Node: v16.19.0 (npm v8.19.3)

To reproduce:

Install flying-squid and run node examples/basic.js to set up a server, and prismarine-web-client from the repo as of today (commit d82a70a5adb0bd514a2141e211ed9661455394db ).

git clone https://github.com/PrismarineJS/flying-squid.git
cd flying-squid/
npm install
node ./examples/basic.js
git clone https://github.com/PrismarineJS/prismarine-web-client.git
cd prismarine-web-client/
npm install
npm start

Connect pwc to fs and move forward constantly, taking memory heap snapshots in Chrome every five minutes.

image

The heap never gets smaller, only bigger. If you run fs with a higher base level for the land the effect is more pronounced (set it to 150, say, and you can build up 2Gb of heap pretty quickly). If you run it for a long time (I used xdotool sleep 3 keydown w+space) after a while some chunks fail to render and then the window crashes with an out of memory error. (Aw snap!)

kf106 commented 1 year ago

To reproduce more quickly, rebind sprint key to Q, and use bot.physics.sprintSpeed=0.9 in the console to move through the world more quickly (I initially produced the error the slow way).

kf106 commented 1 year ago

I've added a repo to make it easier to reproduce the error: https://github.com/kf106/pwc-memleak.git

extremeheat commented 1 year ago

open for PR

kf106 commented 1 year ago

open for PR

I don't understand what you mean.

kf106 commented 1 year ago

Added a console.log() to note when worldrenderer disposes a chunk. The following shows that mesh 32,0,-32 was disposed: image

  removeColumn (x, z) {
    delete this.loadedChunks[`${x},${z}`]
    for (const worker of this.workers) {
      worker.postMessage({ type: 'unloadChunk', x, z })
    }
    for (let y = 0; y < 256; y += 16) {
      this.setSectionDirty(new Vec3(x, y, z), false)
      const key = `${x},${y},${z}`
      const mesh = this.sectionMeshs[key]
      if (mesh) {
        console.log("Removing mesh due to column unload - mesh is at " + key)
        this.scene.remove(mesh)
        if (mesh.geometry) mesh.geometry.dispose()
        if (mesh.material) mesh.material.dispose()
      }
    }
  }

But devtools in Chrome, Memory, snapshot shows that it's still there in system / JSArrayBufferData: image

kf106 commented 1 year ago

The problem is in prismarine-viewer, with worldrenderer.js, primitives.js and entities.js not cleaning up Threejs objects properly. By adding a more thorough dispose function, calling that to remove threejs objects instead of .dispose, and removing references to the objects that go out of view, the system heap size stops growing.

dispose.js:

 const THREE = require('three')

function dispose3 (o) {
    try {
        if (o && typeof o === 'object') {
            if (Array.isArray(o)) {
                o.forEach(dispose3);
            } else
            if (o instanceof THREE.Object3D) {
                dispose3(o.geometry);
                dispose3(o.material);
                if (o.parent) {
                    o.parent.remove(o);
                }
                dispose3(o.children);
            } else
            if (o instanceof THREE.BufferGeometry) {
                o.dispose();
            } else
            if (o instanceof THREE.Material) {
                o.dispose();
                dispose3(o.materials);
                dispose3(o.map);
                dispose3(o.lightMap);
                dispose3(o.bumpMap);
                dispose3(o.normalMap);
                dispose3(o.specularMap);
                dispose3(o.envMap);
            } else
            if (typeof o.dispose === 'function') {
                o.dispose();
            } else {
                Object.values(o).forEach(dispose3);
            }
        }
    } catch (error) {
        console.log(error);
    }
}

module.exports = { dispose3 }
extremeheat commented 1 year ago

Doesn't seem right to need a function like that. Are you sure this is intended usage of threejs? Perhaps it's a bug in threejs not one in prismarine-viewer? Dispose on material/geometry/texture should be all that's necessary.

https://threejs.org/docs/#manual/en/introduction/How-to-dispose-of-objects https://github.com/mrdoob/three.js/blob/master/examples/webgl_test_memory.html https://github.com/mrdoob/three.js/blob/master/examples/webgl_test_memory2.html

kf106 commented 1 year ago

It might be a bug in Three. I tried removing just geometry and material in all places I could find, but that didn't work. The removal function did work - it also removes references. I don't have time at the moment to replace it with a simpler removal function.