Skip to content
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

Memory leak in prismarine-web-client #333

Closed
kf106 opened this issue Jan 9, 2023 · 8 comments
Closed

Memory leak in prismarine-web-client #333

kf106 opened this issue Jan 9, 2023 · 8 comments

Comments

@kf106
Copy link
Contributor

kf106 commented Jan 9, 2023

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 d82a70a ).

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
Copy link
Contributor Author

kf106 commented Jan 9, 2023

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
Copy link
Contributor Author

kf106 commented Jan 9, 2023

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

@extremeheat
Copy link
Member

open for PR

@kf106
Copy link
Contributor Author

kf106 commented Jan 10, 2023

open for PR

I don't understand what you mean.

@kf106
Copy link
Contributor Author

kf106 commented Jan 10, 2023

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
Copy link
Contributor Author

kf106 commented Jan 12, 2023

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
Copy link
Member

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
Copy link
Contributor Author

kf106 commented Jan 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants