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

Elimination of manual memory management (by default and/or within an init option) via JavaScript's FinalizationRegistry/WeakRef? #115

Open
josephrocca opened this issue Mar 2, 2024 · 12 comments

Comments

@josephrocca
Copy link

josephrocca commented Mar 2, 2024

Here are the relevant MDN docs:

My understanding is that using FinalizationRegistry, it would look something vaguely like this:

const finalizationRegistry = new FinalizationRegistry(wasmRef => {
  // the JS-level Vec3 instance has been garbage collected, so deallocate the emscripten-level stuff
  deallocateTheThing(wasmRef);
});

export class Vec3 {
  constructor(x, y, z) {
    // create wasmRef, etc ...
    finalizationRegistry.register(this, wasmRef); // this tells the runtime to watch for when `this` is garbage collected, and when it is, call the registry callback (defined above) with `wasmRef` as the argument
  }
  // ...
}

// Note: The same finalization registry instance can be used for the whole Jolt module/engine IIUC.

So that instead of this:

let material = new Jolt.PhysicsMaterial();
let size = new Jolt.Vec3(4, 0.5, 0.5);
let box = new Jolt.BoxShapeSettings(size, 0.05, material);
Jolt.destroy(size);
// ...

We can just write this:

let material = new Jolt.PhysicsMaterial();
let size = new Jolt.Vec3(4, 0.5, 0.5);
let box = new Jolt.BoxShapeSettings(size, 0.05, material);
// ...

If it'd somehow be a non-trivial perf hit or a breaking change, then maybe some sort of init option? (If my opinion is at all useful here, I think it should be default)

I'm currently using Rapier for a project and it's been great in terms of JS idiomaticity (I guess Rust's wasm-pack handles memory stuff automatically?), so I'm just posting this in the hopes that Jolt can get to a similarly pleasant DX. Thanks!

@jrouwe
Copy link
Owner

jrouwe commented Mar 2, 2024

I'm curious what @PhoenixIllusion and @LeXXik think of this.

I'm using the webidl binder script to generate the bindings between JS and C++ and while it gets you going very quickly, it is quite restricting in what it can do. As far as I know you cannot control how the JS bindings are created, so implementing your suggestion probably means scrapping the whole webidl binder and redoing the entire API from scratch. The emscripten documentation mentions using the FinalizationRegistry but I haven't seen it actually do it.

@PhoenixIllusion
Copy link
Contributor

PhoenixIllusion commented Mar 3, 2024

I am trying to think of what cases we'd actually want uncontrolled garbage collection. Almost all the critical information of memory references live on the C++ layer, and the JS layer will not have any knowledge of that information. There are a couple classes it could theoretically work (shape/constraint/body settings), but most in general have super-high risks of doing something unintended or wrong.
Both of those MDN pages say Avoid where possible

The JS doesn't even really 'own' anything. It just is wrapping a numeric pointer that can trivially be passed round as a u32/u64 and rebuilt inside another light-weight WebIDL wrapper as many times as you want with 100% no impact on the underlying WASM layer, and none of those specific JS wrappers creations or destructions should trigger any WASM destroy calls.

Even the task of something like:

function foo() {
  const pointerA = Jolt.createPointerObject()
  wasmItemB.obj = pointerA;
}

WeakRefs would destroy pointerA, even though it is now owned by itemB, because obj = pointerA is faking a backside setter to call something like u32 rawPointer1 = unwrap wasmItemB, u32 rawPointer2 = unwrap pointerA, emscripten_set_wasmItemB(rawPointer1, rawPointer2)

A secondary issue is something where we sometimes randomly do casts that just extract the raw pointers and rewrap them in other JS objects, discarding the first JS class (though webIDL still holds a ref internally, possibly forever):

const constraint = pathConstraintSettings.Create()
const pathConstraint = Jolt.castObject(constraint, Jolt.PathConstraint)

image

WebIDL by default does not work well at all for this technique, no. WebIDL will internally hold pointer refs to anything that you ever alloc'd but not free'd in WebIDL (like BoxShapeSettings) using getCache until destroy is called, and also for lots of the cases of just wrapped pointers as well.

When it's running in production, you will see these pointer storage refs in some extra Dictionary tacked onto the object, like Jolt.Vec3.Fma or Jolt.Vec3.nma, the name varying between compiles. This dictionary means that even if your JS functions have lost scope and a weak-ref could normally trigger, internally the inside of the Jolt library still holds a MemAddress -> Pointer ref that will keep the WeakRef from triggering.

Even if we didn't use WebIDL, I'm just not sure how we could make this work with the current C++ library given how much extra meta-data and control layers you'd need to add to markup every function as 'this method creates a controlled JS object, this one assumes control of it, this one returns a static, non-memory controlled item, this one deletes 3 or more memory-controlled items or an entire pool of cascading deletes on it's own (ex: destroy of a BodyManager)'. Possibly that is approaching the level of writing a custom GC framework on it's own.

Possibly if we had a way to extract that data we could capture 80%+ of it through a scrape of the code and some Bison/ANTLR language-parser, but for memory management of freeing, you really can't live with the extra 20 messing up and triggering a double-free and app crash.

@LeXXik
Copy link
Contributor

LeXXik commented Mar 3, 2024

Not sure, if there is much to add, but I personally think it is not viable. At least not today. It works well in Rapier, because wasm_bindgen supports WeakRefs and FinalizationRegistry. In contrast, webidl_binder (which is in use currently) doesn't. The only way would be to scrap it and move to Embind (even then no strong guarantees), but it has a downside of having a high runtime overhead compared to webidl_binder. This is the main reason Ammo has decided against using it. And if you ask me to chose between DX and performance, I pick performance over DX any time of the day.

@josephrocca
Copy link
Author

josephrocca commented Mar 3, 2024

@LeXXik Do you know if the performance overhead of Embind is practically relevant? This emscripten doc says it's 200ns per call (with the implication that WebIDL is less than this), but that doc was last edited a couple of years ago.

If Jolt is making thousands of calls into wasm every frame, then this could have a non-trivial performance impact, but I'm guessing it's a lot less than that?

Also seems like work has since been (and is continuing to be) done with Embind performance. Wondering if @RReverser might have any comment on performance implications of switching from webidl_binder to Embind for a project like Jolt? (Apologies for the ping if not - please ignore.)

@LeXXik
Copy link
Contributor

LeXXik commented Mar 3, 2024

I haven't measured it, but trust Ammo made an educated decision. Perhaps, @kripken would offer an insight on his experience.

However, if you know that an object might not be garbage collected, would you still not manually destroy it? No strong guarantee is just that. Same as no GC, if you ask me.

@josephrocca
Copy link
Author

josephrocca commented Mar 3, 2024

Yep definitely interested in kripken's thoughts here!

Same as no GC, if you ask me.

You're basically saying that the lack of spec-level timing guarantees1 means that GCs are an inherently useless technology? If so, I think the whole JS ecosystem disagrees :)

Or are you saying that Embind provides no mechanism at all for explicit freeing of memory? E.g. in case there are unpreventable allocations in a hot path that could do with an explicit free to prevent GC pauses? If that is the case, I guess it also goes in the "is this practically relevant" bucket, since even then the solution tends to just be "find a way to not allocate in hot path".


1 RE the "not at all" from this MDN quote: "When a registered object is reclaimed, any cleanup callbacks for it may be called then, or some time later, or not at all." - I don't think this should be taken to mean anything other than "There is very literally no GC implementation detail that is encoded in the spec / visible to userland." Someone can correct me if I'm wrong here, but I think it's a technical/theoretical point, rather than something that is practically relevant (to devs who understand not to depend on timing -- in e.g. unit tests or whatever).

@PhoenixIllusion
Copy link
Contributor

One potential path is to split the difference between "recode for auto Gc in different Em framework" and the "webIDL default", by creating a helper library (optional for people to import) along side the main library that takes the commonly used classes that actually need GC and making a factory for them with this wrapper tacked on.

It would still be up to a user to know if it's safe to use that version since I'm not sure on edge cases of deleting some shape/constraint settings 100% on weak-ref, given I don't know if Jolt has cases it might assume ownership via a method call. Even EmBind seems to have the same problems without some additional work.

I'm still thinking through the gains vs cost of the auto-GC, on this project specifically, when it seems to primarily impact Settings objects and some of the core Math classes (Vec,Quat). Is it mainly just for those classes? The Jolt internal ref-counting is handling quite a bit behind the scenes once you've actually created a constraint/shape/body and passed it into the physics system for ownership.

On the math side, personally use 3-4 vectors and maybe 2 quat in my entire library and a heavy use of Vec3.Set and Quat.Set, and so do some of the 3d engines I've seen (a TempVector pattern)

@PhoenixIllusion
Copy link
Contributor

PhoenixIllusion commented Mar 4, 2024

Trivial Proof-of-Concept of an AutoGC helper class - main...PhoenixIllusion:JoltPhysics.js:feature/auto-gc-tool

Heavy console Logging present so I could check the execution paths.

I added a WeakRef + Timer fallback which may be a horrible idea. Uses a generic AutoRef method that blindly Jolt.destroy's anything you hand it, and wraps it in transparent-proxy to create a weak-ref-able object with no possibly outside pointers in the core Jolt library, and then passes it to the FinalizationRegistry (or fallback). I added some simple constructor wrappers at the bottom.

I've never touched the FinalizationRegistry before so have no clue if this approaches a prod-ready approach or any unknown hazards and cross-browser-support issues. I don't think I'd trust it in my actual job-work on a long-running process unless I had a couple test machines and examples/browser setups and ran a stress-test for a couple hours each.

@kripken
Copy link

kripken commented Mar 4, 2024

Regarding embind/WebIDL binder performance, I believe even today the WebIDL binder is faster, even with recent improvements on embind - embind is just more flexible, and there is a tradeoff there.

With that said @brendandahl is working to optimize embind and may have other thoughts here. In particular if embind gains a fully "static" mode where the bindings do no dynamic lookups at all, that seems like it could be as fast as the WebIDL binder (when one uses the same simple types as the WebIDL binder supports). @brendandahl ?

@brendandahl
Copy link

WebIDL is still be faster today. For functions function exports I think the two can be made the same speed with some improvements to embind. For most workloads though, it seems the overhead of the binding layer is tiny compared to the work that is actually happens in Wasm. I've also seen some projects use embind for most of the API and if there is a really hot code path, use a plain C function export for that.

@MAG-AdrianMeredith
Copy link

I've done something similar in another wasm project and although its not currently merged, it did work quite nicely. IIRC references were tracked wasm side and so the finalization registry allow us to notify that the js side no longer needs the value. Saved a ton of memory bugs and much simpler code for devs

@josephrocca
Copy link
Author

I'm not sure if this is helpful at all - from latest version release notes:

Embind now supports return value policies to better define object lifetimes. See https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html#object-ownership for more information.

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

7 participants