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

Work Out Ref Garbage Collection / Cleanup #16

Closed
zicklag opened this issue Aug 27, 2022 · 5 comments · Fixed by #19
Closed

Work Out Ref Garbage Collection / Cleanup #16

zicklag opened this issue Aug 27, 2022 · 5 comments · Fixed by #19

Comments

@zicklag
Copy link
Contributor

zicklag commented Aug 27, 2022

Opening this issue to continue the discussion about the lifetime of refs here.

In summary, on native, we can hook into the finalization to clean up our references on the Rust side, when the JavaScript object is garbage collected, but in the browser that doesn't work because we don't have destructors in JS.

In #11 this was handled in the browser by deleting all of the refs on the Rust side every frame, invalidating any refs in the JavaScript side that are persisted across frames.


I just had an idea, though, where maybe we could default to deleting the ref every frame, but allow you to call .persisted() on the ref to make it stick around across frames. Then you would have to later call .free() to have it deleted at the end of the next frame.

That might give you a little more flexibility, letting you opt-in to long-lived refs if you want to, but without complicating the default case of having refs deleted at the end of every frame.

@jakobhellermann
Copy link
Owner

Maybe we can use the ,FinalizationRegistry to clean up resources on the rust side: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry

@zicklag
Copy link
Contributor Author

zicklag commented Sep 3, 2022

Good find! That looks extremely promising. That looks like that would work.

I've got some work that might take a week or more to finish up, but once I can get back to this I'd definitely be willing to look into that, assuming you don't want to get to it first. I don't care at all either way.

@zicklag
Copy link
Contributor Author

zicklag commented Sep 20, 2022

I'm getting some really strange behavior when testing out the finalization registry.

I've just got a tight loop that calls this function 10,000 times per frame:

        test(n) {
            const test = new WeakRef({ message: `hello world ${n}` });
            trace(test.deref());
            // globalThis.valueRefFinalizationRegistry.register(test, n);
        }

With the code above, the memory usage of the game is as expected. Since the test variable is only used in that scope and it can be discarded right after the function call.

But if I uncomment the commented line, which should simply register a callback to run when the object is garbage collected, the memory usage grows incredibly fast.

It's like the finalization callback isn't running at all and, in fact, is causing the test variable to be kept forever!

I'm going to need to do some more investigating.

@jakobhellermann
Copy link
Owner

Hm, if the FinalizationRegistry doesn't end up working (it is called out as unreliable in the official docs after all) clearing the map after every frame would be a valid option as well.
It would more closely match the host model where you can't have live references across frames either, you need to fetch it every frame if you want to use it.

@zicklag
Copy link
Contributor Author

zicklag commented Sep 20, 2022

Ah, I figured it out! I needed to run the JS runtime event loop, which makes a lot of sense, actually. I was just telling it to run our scripts, and never giving it the opportunity to run the pending garbage collection callbacks.

I just tested it out and it works great! Even when creating thousands of refs per frame, we maintain a consistent memory usage, and the refs are properly garbage collected. While I haven't tested it yet, it should work the same in the browser, too.

I'll get the browser working again next.

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 a pull request may close this issue.

2 participants