Skip to content

Fixed memory leak in OBJLoader for V8 based js engines #9680

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

Merged
merged 1 commit into from
Sep 14, 2016

Conversation

Cobertos
Copy link

See issue #9679

@mrdoob
Copy link
Owner

mrdoob commented Sep 14, 2016

I'll merge by now, but I'll add a note.

@mrdoob mrdoob closed this Sep 14, 2016
@mrdoob mrdoob reopened this Sep 14, 2016
@mrdoob mrdoob merged commit f34e136 into mrdoob:dev Sep 14, 2016
@mrdoob
Copy link
Owner

mrdoob commented Sep 14, 2016

Thanks!

@jonnenauha
Copy link
Contributor

jonnenauha commented Sep 14, 2016

Cool find, I'll try to check at some point if this has any impact on the perf. One particular perf problem is GC pauses while parsing large OBJ files. Not entirely sure if this will make it worse or better. It could now have to collect more stuff as it's no longer leaking, hard to say though without testing :) But ofc a good fix, leaking permanently on the page is worse than one time executed loads (if this even had impact on the load time).

How big was the obj file you were testing? lines or file size? Did you see something significant in the mem profile after the fix? What kind of figures are we talking about?

@Cobertos
Copy link
Author

The .obj is 37MB, 1.02 million lines. The strings would collect between an "invocation" of a specific component in our application that uses the WebGLRenderer. Even after deleting the canvas and disposing() of the objects there would still be one copy of this string in memory which piled up after a couple view changes. It was enough that I was getting 1GB profiles after around 10 or so changes and it was crashing the app.

aardgoose pushed a commit to aardgoose/three.js that referenced this pull request Oct 7, 2016
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

Successfully merging this pull request may close these issues.

3 participants