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 when using reloadDocument() #185

Open
sdumetz opened this issue Feb 9, 2023 · 13 comments
Open

Memory leak when using reloadDocument() #185

sdumetz opened this issue Feb 9, 2023 · 13 comments

Comments

@sdumetz
Copy link
Contributor

sdumetz commented Feb 9, 2023

Some memory isn't released when changing documents. It's especially visible with large models as hundreds of MB go missing on each reload.

Using WebGLRenderer.info, I see increased textures and geometries usage over time.

Calling WebGLRenderer.dispose() on each document change releases the memory properly. Using this ugly hack confirmed normal memory usage for me :

diff --git a/source/client/ui/SceneView.ts b/source/client/ui/SceneView.ts
index 7894cbe..9c84ced 100644
--- a/source/client/ui/SceneView.ts
+++ b/source/client/ui/SceneView.ts
@@ -117,6 +117,11 @@ export default class SceneView extends SystemView
         this.splitter.layout = EQuadViewLayout.Single;
 
         this.manipTarget.next = this.view;
+        this.system.getMainComponent(CVDocumentProvider).on("active-component", (e:any)=>{
+            if(!e.next){
+                this.view.renderer.dispose();
+            }
+        }, this);
     }
 
     protected connected()

calling dispose() on the renderer is vaguely documented over the internet but it's generally recommended when the renderer is not to be used anymore... It doesn't cause any trouble here but still feels wrong. It might be possible to release memory without calling renderer.dispose() but I couldn't find it.

If renderer.dispose() is really required, there might be a good place to call it?

I'd like some advice on this to find a better solution.

@gjcope
Copy link
Collaborator

gjcope commented Feb 9, 2023

Hi @sdumetz - Three.js doesn't release the geometry/material/texture resources automatically due to possible shared use so that has to be done manually by calling dispose on each, though we are doing that and it seems to be working as expected. When I profile in Chrome and switch between scenes I'm seeing the allocations released when the new scene is loaded. There could be other unreleased resources tied to WebGLRender, but if you are seeing hundred of MB I would assume it is model related as I am seeing nothing like that.

Do you have any example models you can share that replicate the problem so we can be looking at the same data?

@gjcope
Copy link
Collaborator

gjcope commented Feb 9, 2023

Also FWIW I tried your code above (calling dispose on the renderer) and I'm not seeing any difference in memory released. Are you doing anything custom with model loading/rendering in your fork?

@sdumetz
Copy link
Contributor Author

sdumetz commented Feb 9, 2023

Not doing anything too fancy with loading or rendering (or shouldn't be, code is not entirely mine and still a bit messy).

Platform is electron on linux, it should work like Chrome.

Models are ~1M poly each with a large texture.

The code is here https://github.com/Holusion/e-thesaurus/tree/main/source/holusion but it's a bit of a mess. I ruled out a problem on my part as I was importing ExplorerApplication as-is, but it might be worth it to double check. I'll try to put together a minimal reproduction case to be sure.

@sdumetz
Copy link
Contributor Author

sdumetz commented Feb 10, 2023

So I was able to reproduce (again, on electron/linux) with a minimal example :
https://github.com/Holusion/e-thesaurus/blob/memory_leak/source/holusion/client/MV2.ts

If you want to run in on electron, you can clone this branch from my repository, the build process looks like :

npm i
cd source/holusion
npm i
npm run build
npm start

Otherwise just copying the file over MainView.ts should more or less work. Excepted if it's an electron-only issue, obviously.

I used dummy models to be sure there is nothing special with the ones I'm usually using and though they are lighter, the leak is clearly there. You can DL them here : https://files.holusion.net/scenes.zip

Surprisingly electron doesn't report a growing JS memory usage. I even checked for proper GC of the ArrayBuffers to be sure. It's the system memory (as reported by htop) which doesn't get released.

@gjcope
Copy link
Collaborator

gjcope commented Feb 10, 2023

Thanks! I will check it out asap.

@gjcope
Copy link
Collaborator

gjcope commented Feb 13, 2023

I haven't had a chance to build your branch yet, but did dig in to the issue a bit. I did find one small leak relating to shadow casting lights. The associated render target is not currently being disposed appropriately. But that's a relatively small issue unless you are using a lot of shadow casting lights.

Using WebGLRenderer.info, I see increased textures and geometries usage over time.

I too saw increasing textures due to the shadow leak mentioned above. WebGLRenderer.info did not reflect increasing geometry usage.

Calling WebGLRenderer.dispose() on each document change releases the memory properly. Using this ugly hack confirmed normal memory usage for me :

This is the most confusing for me. I am not seeing this at all. I do see the Chrome process use increasing memory when reloading, but also sometimes it drops on reload - we don't really have control over that. I see no difference in that whether I directly call dispose on the renderer or not. I'm seeing very little js heap memory not being released when profiling.

@sdumetz
Copy link
Contributor Author

sdumetz commented Feb 14, 2023

new data and precisions

I'm using the default scene 3-points lights so I don't think it's the issue.

I'm seeing little heap memory too (10-15MB max.) and no visible leak here.

I finally managed to make a memory-infra trace(attached).

It's a run where models are swapped every 5s for 1 minute, in which Renderer memory grows gradually from 70MiB to 2 600 MiB.

The trace tells me it's primarily malloc memory given to directMap objects, which doesn't help much.

I couldn't get more data on what holds this memory but the only thing not in JS space that could take such space would be WebGLBuffer and associated classes. Unfortunately I couldn't verify this formally.

additional tests

So I looked into WebGLRenderer.dispose() and it wasn't very useful. Most functions just reset a WeakMap which can't be holding memory in the first place.

I really don't know what is wrong here.

@sdumetz
Copy link
Contributor Author

sdumetz commented Feb 14, 2023

OK so I tested some more and it's programCache.dispose(); that clears the memory.

the only thing it does is clearing the WebGLShaderCache.js which in turn clears a shaderCache and materialCache. Both of which are NOT a WeakMap.

However I can't even find what they are used for.

@gjcope
Copy link
Collaborator

gjcope commented Feb 14, 2023

Thanks for the investigation and extra info. I will look further into the caches and see if I can provide any more insight. My main concern is that I still cannot reproduce the behavior of WebGLRenderer.dispose() (or the included cache dispose) having any effect on the memory usage. I haven't had time to deploy your code yet, but looking at it my guess is it is because you are creating a new view (which creates a new WebGLRenderer instance) each time you swap the model. If possible for your use case, I would keep the renderer around and just swap the content.

This should still be supported and clean up appropriately though but will have more to do with disposing a view. I will take a closer look at this.

@sdumetz
Copy link
Contributor Author

sdumetz commented Feb 14, 2023

It's a good guess but apparently no : I changed the code to create a ContentView only once in connectedCallback like this :

let container = document.createElement("DIV");
let contentView = new ContentView(this.application.system);
container.appendChild(contentView);
this.shadowRoot.appendChild(container);

From there I just call reloadDocument in a loop and still see the same thing happening.

Just to be extra sure, I just did this in the unmodified explorer code :

diff --git a/source/client/ui/explorer/MainView.ts b/source/client/ui/explorer/MainView.ts
index 6a9eab4..1d85ef3 100755
--- a/source/client/ui/explorer/MainView.ts
+++ b/source/client/ui/explorer/MainView.ts
@@ -125,6 +125,14 @@ export default class MainView extends CustomElement
         this.attachShadow({mode: 'open'});
         const shadowRoot = this.shadowRoot;
 
+        let documents :string[]= [ /*insert two scenes path here*/];
+        let index = 0;
+        setInterval(()=>{
+            index = (index + 1) % documents.length;
+            this.application.props.root = documents[index];
+              this.application.reloadDocument();
+          },5000);
         // add style
         var styleElement = document.createElement("style");
         styleElement.innerText = styles;

and see the same increasing memory usage over time (on Chromium/Linux).

@sdumetz
Copy link
Contributor Author

sdumetz commented Feb 14, 2023

Even simply this.application.reloadDocument() in a loop does it. You need a big model for it to be noticeable in system memory, obviously.

This might be simpler for you to troubleshoot than having to build my application.

@gjcope
Copy link
Collaborator

gjcope commented Feb 14, 2023

Ok, I will give that a try. I do see the increasing memory usage over time, but also see it eventually drop back down and again not seeing any impact from renderer.dispose().

@gjcope
Copy link
Collaborator

gjcope commented Mar 21, 2023

I added your code above as suggested and monitored all of the memory usage of all of the processes launched by the browser. You can see the spikes every 5 sec, but eventually it drops back down to the expected level so I'm not really able to reproduce the issue.
image

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