-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
Dispose of loaders #20958
Comments
DRACOLoader does have a Given that the usage pattern is... var loader = new GLTFLoader().setDRACOLoader( dracoLoader ); ... do you think GLTFLoader should still have a |
Didn't know that about DRACOLoader - thanks. Draco is challenging to deal with because of the way its multiple script assets are configured and loaded.
Yes, I think this is the key point here. There are potentially resources besides workers that need to be cleaned up, and those will shift over time. The cleanest, clearest and safest way is to encapsulate that into a common dispose method that we can count on to be there. I think it's not such a stretch, since it already exists on many other objects in this library, like materials and geometries. As a general rule, I think it's good practice that an object should be responsible for objects that it creates. You shouldn't have to dig inside to clean up after it. There's an assumption that resource management isn't something that makes sense in a garbage collected language like JS, but that's not practical in reality when you're working with something as stateful as WebGL and a big framework like three.js. Dispose/destroy is a pattern that I've been relying on more and more in my own code and I'm loving it. |
Do you have specific examples of what resources you're expecting to be cleaned up? As far as I know OBJLoader2Parallel (which automatically terminates its worker) and DRACOLoader are the only two that create objects that can potentially be cleaned up (Web Workers / WASM). Everything else should naturally fall out of scope and be collected by the GC -- there wouldn't be value anywhere else.
I think it would be nice to cancel requests, as well, but disposing of a Loader doesn't feel like the right way to do this to me. Maybe setting an AbortSignal on the loaders would be a good way to enable this and be future proof to if / when FileLoader switches to use |
Agreed, but GLTFLoader instances don't create their DRACOLoader and KTX2Loader dependencies, so the user would need to dispose all three in this scenario. That's still probably for the best I think.
BasisTextureLoader also creates web workers, and KTX2Loader eventually will (but doesn't yet). |
I'm looking at refactoring KTX2Loader and DRACOLoader a bit. Both would have a WorkerPool, which manages Web Worker tasks. Because the two loaders execute different tasks on the worker, they can't currently share a WorkerPool instance. The harder question seems to be: If multiple instances of e.g. KTX2Loader are created, should they share a WorkerPool?
I'm tempted to just show a warning when multiple KTX2Loader or DRACOLoader instances are active (i.e. not disposed) at the same time. This way no one is caught by surprise by the performance penalty, and there isn't a lot of weird shared state behind the scenes. The only real use case I can think of that requires multiple KTX2Loader instances would be comparing different transcoder versions, and that's just a debugging task where warnings can be safely ignored. Related: #22445 |
Proposal for KTX2Loader: #22621 |
I think if One thought that comes to mind is whether we need to require the user to completely "dispose" of the WorkerPool at all? Can worker instances be created as needed (up to some user-settable cap) and be automatically terminated as the number of jobs goes down? Or be terminated after a user-settable number of seconds passes and they haven't been used? Or of course the user can explicitly terminate the workers on command if needed. WeakRef comes to mind, as well, but I'm not sure if that would afford anything unique with GC that the browser isn't already doing with WebWorkers. The 3DTilesRendererJS project has a related pattern maybe worth mentioning. In order to manage many downloads and tile file parsing every const tilesRenderer = new TilesRenderer( './path/to/tileset.json' );
const tilesRenderer2 = new TilesRenderer( './path/to/tileset2.json' );
// set the second renderer to share the cache and queues from the first
tilesRenderer2.lruCache = tilesRenderer.lruCache;
tilesRenderer2.downloadQueue = tilesRenderer.downloadQueue;
tilesRenderer2.parseQueue = tilesRenderer.parseQueue; |
Is your feature request related to a problem? Please describe.
Often, loaders for various file formats (e.g. GLTFLoader, ImageLoader, etc.) are used early in a site life cycle and then not needed anymore, but sometimes they leave resources in use that could be cleaned up. Specifically, loaders that use web workers will leave those threads running in the background.
Describe the solution you'd like
I'd like to see modern and widely used loaders have a
dispose
method that cleans up any existing resources, like running web workers or files cached in memory. Ideally, any downloads in progress will be aborted. I see FileLoader still uses XMLHttpRequest, which has broad support for.abort
.Describe alternatives you've considered
We could expose underlying resources on the different loader objects, but that would be unwieldy and brittle since each loader would have different resources.
Additional context
This seems related to #18234 and possibly dependent upon #19650. I don't see this specific ability mentioned in that issue or in that pull request in progress, so I figured I'd file it separately.
Thank you.
The text was updated successfully, but these errors were encountered: