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

useLoader() does not dispose of loader instance (causes memory leak with KTX2Loader) #2812

Closed
Olliebrown opened this issue Mar 17, 2023 · 0 comments · Fixed by #2984
Closed
Labels
bug Something isn't working dependencies An upstream issue to do with dependencies

Comments

@Olliebrown
Copy link

Expected Behavior

The useKTX2() hook (from drei), which is a wrapper for the useLoader() hook, should either reuse the existing loader or properly dispose of the loader once loading is finished. This is important for the KTX2Loader because it allocates web workers to do the transcoding and these are not de-allocated if you do not call dispose (see KTX2Loader:dispose() docs)

Observed Behavior

The useKTX2() hook calls useLoader() which creates a new KTX2Loader on each call and vicariously spawns new basis transpiler threads. These are never deallocated and start to leak into stack memory for the browser. After enough calls, the browser (chrome in my case) runs out of memory and crashes.

Possible Cause

I dug through both useKTX2() and useLoader() and I believe the problem has to be addressed inside useLoader() (hence me posting this here instead of in drei). It creates a new instance of a loader, uses it to load all the indicated textures (inside a Promise.all), and then resolves once all are loaded. The reference to the loader is left dangling and never gets properly disposed.

Because the reference is abandoned, useKTX2() does not receive it and cannot call dispose() itself. I think that userLoader() would either need to return the loader instance it creates or dispose of it for you.

Workaround

I duplicated the useLoader() method and its partner loadingFun() and adjusted them so that the Promise that is created has a finally() added to it that disposes of the loader.

The important change is inside of loadingFun() and looks roughly like this:

// Go through the urls and load them
const promiseGroup = Promise.all(
  // ... snip, remains unchanged ...
)

// Dispose of the loader when the promise is done
promiseGroup.finally(() => loader.dispose())
return promiseGroup

I do not know typescript so I just stripped out all the types and hacked this together for my own purposes. This means I'm really not in a position to offer a pull request for this. This is also the first time I've tried diving into the internals of R3F and I could easily be underestimating the consequences of this change for other loaders. It works for me with my kltx2Loader situation but I haven't tested it for other loaders. I could also anticipate it might be better to find a way to enable the useKTX2() hook in drei to take care of this, but near as I can tell, this is not possible the way useLoader() is currently implemented.

How I Found This

I have been utilizing ktx2 compression to keep the size of 8k 360 pano images down in a virtual tour application I am developing. This results in a pattern like this:

  • When entering a room, load the texture for that room and all adjacent rooms
  • When moving rooms, more texture loading naturally occurs (for any new adjacent rooms)

When these were NOT ktx2 textures, this pattern worked fine. Once I switched to ktx2 textures, I started getting warnings from the ktx2Loader from three.js and, after entering enough rooms, chrome was running out of stack memory and crashing.

@CodyJasonBennett CodyJasonBennett added the bug Something isn't working label Mar 17, 2023
@CodyJasonBennett CodyJasonBennett added the dependencies An upstream issue to do with dependencies label Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies An upstream issue to do with dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants