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

Fix the issue that memory for disposed objects is not released #12464

Open
wants to merge 5 commits into
base: dev
from

Conversation

Projects
None yet
7 participants
@takahirox
Copy link
Collaborator

takahirox commented Oct 22, 2017

This PR fixes #12447

Let me know if there's any special reasons that renderItems wasn't cleared in init()

@aardgoose

This comment has been minimized.

Copy link
Contributor

aardgoose commented Oct 22, 2017

Hi,
RenderItems isn't cleared in init() because it acts as a cache of RenderItem objects rather than having repeated allocations for each frame (init is called every frame). This patch will cause excessive heap churn and GC.

@aardgoose

This comment has been minimized.

Copy link
Contributor

aardgoose commented Oct 22, 2017

I came across this issue hence renderLists is exposed by the renderer so you can do:

renderer.renderLists.dispose();

to drop all the references to objects, materials etc after removing a set of objects or finishing with a camera / scene combination. Not an elegant solution but it works, and allows the reductions in per frame heap allocs introduced by renderLists to be maintained.

@takahirox

This comment has been minimized.

Copy link
Collaborator Author

takahirox commented Oct 23, 2017

You mean, array.length = 0 allocates new memory?

And I don't think renderer.renderLists.dispose(); is a good idea. I don't want users to be aware of renderer.renderLists for object disposal.

@takahirox

This comment has been minimized.

Copy link
Collaborator Author

takahirox commented Oct 23, 2017

Ah, you meant allocating new memory here if we clear renderItems, don't you.

https://github.com/takahirox/three.js/blob/871abf5cd090a67c5373bbdbf67f4dd7532d77ba/src/renderers/webgl/WebGLRenderLists.js#L73-L82

OK, I need to update not to allocate new memory each frame...

takahirox added some commits Oct 23, 2017

@takahirox

This comment has been minimized.

Copy link
Collaborator Author

takahirox commented Oct 23, 2017

Updated

@aardgoose

This comment has been minimized.

Copy link
Contributor

aardgoose commented Oct 23, 2017

Correct, that's the allocation the render lists caches.

Don't know what the performance hit is of clearing all the references, in most cases, with an unchanged number of render items between frames you could probably avoid most of this clearing by using renderListIndex as a high water mark from the previous frame and clear from renderListIndex to renderList.length - 1 : That is, the RenderItems that haven't been overwritten in the previous frame.

I think this would in the common case avoid most of the overhead?

The renderLists.dispose() function is the only answer in some cases where a camera/scene combination won't be reused, but yes it is a very blunt object and a bit ugly except in special cases ( I required it after a rtt pass.

@takahirox

This comment has been minimized.

Copy link
Collaborator Author

takahirox commented Oct 23, 2017

Updated.

I think having WebGLRenderList.finish() again like r86 and clearing references in it would be more straightforward.

@mrdoob Let me know if you don't wanna have WebGLRenderList.finish() again.

@Mugen87

This comment has been minimized.

Copy link
Collaborator

Mugen87 commented Jan 4, 2018

We should merge this PR or a similar solution. There are scenarios where WebGLRenderList effectively avoids garbage collection, see https://jsfiddle.net/v6bbxxa1/

Although 90 of the 100 objects are removed, the internal renderItems array of WebGLRenderList still has 100 entries with references to obsolete geometries. Something like WebGLRenderList.finish() would free this stuff.

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 3, 2018

This is how the code used to be before. I changed it because, depending on the scene, it would be creating and deleting objects all the time.

Would using WeakMaps help here?

@aardgoose

This comment has been minimized.

Copy link
Contributor

aardgoose commented Mar 3, 2018

@mrdoob
You sadly couldn't replace the list with a WeakMap. You can't iterate them which is the price of weak references + keys must be objects, so the string 'hash' wouldn't work.

@mrdoob mrdoob modified the milestones: r91, r92 Mar 14, 2018

@anzorb

This comment has been minimized.

Copy link

anzorb commented Apr 12, 2018

Our application suffers from this in production and we are seeing GPU process crashes due to a memory leak. Running renderer.renderLists.dispose() makes the profile look clean again. How far is this PR from being complete/released and what do you recommend doing in the meantime? When is a good time to run renderer.renderLists.dispose()? Thanks!

EDIT: looks like updating to r91 has resolved this issue. I'll continue to monitor.
EDIT: looks like there were still Arrays left behind, that are referencing Geometries that were disposed. Calling renderer.renderLists.dispose() after bulk removal of meshes looks to be the solution for now.

@mrdoob mrdoob modified the milestones: r92, r93 Apr 18, 2018

@mrdoob mrdoob modified the milestones: r93, r94 May 22, 2018

@mrdoob mrdoob removed this from the r94 milestone Jun 27, 2018

@mrdoob mrdoob added this to the r95 milestone Jun 27, 2018

@clanehin

This comment has been minimized.

Copy link

clanehin commented Jul 7, 2018

This patch will cause excessive heap churn and GC.

I changed it because, depending on the scene, it would be creating and deleting objects all the time.

I feel like minimizing GC activity can be a dubious tradeoff. If the objects being rendered onscreen are the only objects in the heap (and this is very possible now with web workers and web assembly), then the worst-case scenario of running the GC on every frame only visits the same objects that the renderer iterates every frame anyway.

That may be an unnecessary expense, but on the other hand, a memory leak that forces the tab to crash or the OS to swap (as I've been experiencing) is absolutely devastating to the user experience.

@Mugen87

This comment has been minimized.

Copy link
Collaborator

Mugen87 commented Jul 16, 2018

This is how the code used to be before. I changed it because, depending on the scene, it would be creating and deleting objects all the time.

The new code is different than before. finish() does not invalidate render items anymore but only their references to other objects. In this way, the mentioned creation/deletion of objects should not happen. BTW: This is how finish() looked before:

function finish() {
opaque.length = opaqueLastIndex + 1;
transparent.length = transparentLastIndex + 1;
}

I think the approach of this PR is better than the current implementation since it prevents a clearly unexpected behavior of the library when disposing objects.

@mrdoob mrdoob modified the milestones: r95, r96 Jul 31, 2018

@mrdoob mrdoob modified the milestones: r96, r97 Aug 29, 2018

@mrdoob mrdoob modified the milestones: r97, r98 Sep 25, 2018

@mrdoob mrdoob modified the milestones: r98, r99 Oct 31, 2018

@mrdoob mrdoob modified the milestones: r99, r100 Nov 29, 2018

@mrdoob mrdoob modified the milestones: r100, r101 Dec 28, 2018

@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019

@Michalzr

This comment has been minimized.

Copy link

Michalzr commented Feb 20, 2019

Hello,

I've been investigating a memory leak in our application that uses THREE.js. The leak wasn't solved even after I used the changes from this PR.

So what's the problem?

  1. render the scene with camera A
  2. dispose some of the used geometries
  3. render the scene with camera B (and never use camera A again)
  4. Memory leak -> the geometries are stored in render lists, and will never be removed

The obvious solution for me, is not to use new instance of camera (or scene) once I used first one.
So I'm going to do that. But I don't think this is intuitive for THREE.js user. Or is it?

Why are the render lists cached based on camera and scene id?

This is webgl_test_memory project, modified in a way that new instance of camera is created every frame.
Obviously this is not a real life example, it's just to prove my point - the memory grows rapidly there.
https://jsfiddle.net/x7Le8pda/

@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.