Add wbind and wbindArray API.

[juj]

This PR improves the detected performance issue from #12733 by adding a JS side wasm table cache.

wbind() and wbindArray() APIs will not work for signatures that have 64-bit integers unless WASM_BIGINT is enabled.

Another benefit of using wbind() instead of recommending users to directly call wasmTable.get() is that it allows one layer of indirection for better ability to refactor - asking users to access the wasmTable variable directly ties our hands down on what kind of compatibility related refactors we can do in the middle.

[sbc100]

Thanks for continuing the work on this.

I general this looks good. I'm not sure about the naming of wbind .. do you rational for choosing that name? We already have a lot of use of the word bind (embind, webidlbind, etc). Is w for wasm or web maybe?

I'd love to come up with a name that is a little more intuitive? Something that tells me that I'm passing a C function pointer and getting a JS function would be good. I don't have any great ideas.. toJSFunc ? ptrToFunc ? funcFromPtr ?

[juj]

I'm not sure about the naming of wbind .. do you rational for choosing that name?

w there means "wasm", i.e. bind to wasm.

I don't have any great ideas.. toJSFunc ? ptrToFunc ? funcFromPtr ?

This is a core API function for one of the most basic usages of Wasm<->JS interop, so I want it to be short and sweet, so no capital combo letters. That way also the danger of this being regressed in refactors in the future will be reduced.

[kripken]

This should integrate with addFunction/removeFunction (which can be used in non-relocatable builds).

[juj]

Updated this PR to latest.

There is one very important semantical clarification/fix I would like to make with this.

Previously, when one was building with WASM_BIGINT enabled, that would mean that the int64 "ABI"/calling convention for dynCall(), dynCall_xxx() and getDynCaller() would change from the i32 pairs ABI to the BigInt ABI, for all user JS code.

That kind of behavior looks like a breaking change to me, because it is not a change that can be done under the hood transparently, but users must update all their JS library code likewise to now call dynCall()s with BigInt objects. Additionally, there is a big update cadence/coordination problem in scenarios where one party is not in 100% control of all JS library code that is fed to the compiler, like the scenario we are in at Unity (we have both our own JS library code, but also Unity users submit JS library code in the form of plugins).

To fix both issues, we need the int64 ABI to behave in a non-breaking manner. I.e. enabling WASM_BIGINT should not all of a sudden break existing JS code accessing old dynCall()s.

Therefore I would propose that the existing dynCall(), dynCall_xxx() and getDynCaller() family of functions shall always call wasm functions using the old i32 pairs convention. This way existing user code will not break. These functions will be available when building with -s DYNCALLS enabled.

To call a wasm function pointer with the BigInt convention, one will use the wbind() and wbindArray() functions. If user is not building with WASM_BIGINT enabled, then it will mean that these functions cannot be used to call a function with i64 signature.

This way we can enable builds that have both -s DYNCALLS=1 and -s WASM_BIGINT=1 enabled, and old Unity game developer users' JS library code will still utilize the dynCall()s i32 pairs path in a backwards compatible manner, and new Unity JS library code can migrate earlier to the BigInt path with wbind().

For clarity, we could consider adding an explicit INT64_ABI=none/i32pairs/bigint/both option, with the meaning:

• none: JS code cannot call any wasm functions that have a int64 in their signature,
• i32pairs: JS code can call i64 function signatures using the dynCall(), dynCall_xxx() and getDynCaller() family of functions. This build flag implies enabling DYNCALLS=1 mode.
• bigint: JS code can call i64 function signatures using the wbind() and wbindArray() family of functions. This build flag implies enabling WASM_BIGINT=1 mode.
• both: Both of the above i32pairs and bigint calling conventions are active at the same time.

Although I am happy with just the current DYNCALLS and WASM_BIGINT flags.

Extra note:

Besides function pointers, we have the same calling convention change issue with int64s in Wasm Module exports. I think currently if one flips WASM_BIGINT to true, this will change the calling convention on all exports, and all user code would need to be fixed in one go to be able to call any of it.

However for Unity's use, we don't have a mechanism where users could access any of the generated Wasm exports (in C# compiler all calls from JS to wasm are taken via function pointers), so for us it is not necessary to implement a migration compatibility path for int64 containing Wasm exports.

Assuming that the CI comes back green, this should be good to land.

[juj]

This should integrate with addFunction/removeFunction (which can be used in non-relocatable builds).

Resolved.

Where did we land on the naming bikeshedding? The idea with wbind, short for "wasm bind", was to parallel the short names ccall and cwrap that look very lean. Behavior is similar to cwrap, but wwrap would not look good, it visually reads like a typo.

The previously suggested names don't feel that good. E.g ptrToFunc is overloads on already very general programming terms and does not disambiguate on the terms it highlights (data vs func pointer, and wasm/js function?). toJSFunc and funcFromPtr also both read very general and I find them easy to forget - also annoying to type with the mix of abbreviated letters with different cases.

wbind for "wasm bind" feels like the best concept I could come up with.

[sbc100]

Great!

I'm pretty happy with this now. I don't love that we have bind in the name but I can live with it.

[sbc100]

I guess this means that wbind cannot be used (at least not in the general case) unless WASM_BIGINT is enabled?
I guess this inherently the case (and we can't offer, for example, wbind32 for such users) because wbind always binds directly the the compiler output whereas dynCall can use binaryen-generated stubs split i64s?

[juj]

I guess this means that wbind cannot be used (at least not in the general case) unless WASM_BIGINT is enabled?

That is right: wbind can be used for non-64-bit signatures in all builds, but only for 64-bit signatures if WASM_BIGINT is enabled.

That might at first sound like a drawback, but in my mind that is explicitly a desirable property. That is because the i32pairs vs BigInt ABI abstraction is not a hidden internal detail, but the caller must be ready to handle the calling convention and rewrite their code to adjust to the right calling convention.

The scenarios where a caller is an agnostic "passer-through" seem to be a very rare and quirky behavior. Such callers would be expected to have to manually write

  function passesThroughIncomingArgsToAWasmFunctionPtr(funcPtr, args) {
#if WASM_BIGINT
    wbindArray(funcPtr)(args);
#else
    dynCall(getTheSigSomewhere, funcPtr, args);
#endif
  }

but I can't really figure out what kind of usage scenario ever wants to do that. The only two cases we currently have of this are

1. the invoke functions: there like you note below, it is actually an internal implementation detail that it just passes args through,
2. embind's $embind__requireFunction. Here I think it is actually a bug/bad behavior that it does this, since this means that when you flip between WASM_BIGINT enabled vs disabled, it will change the 64-bit ABI for all embind using code, leading to the "all caller code sites need to migrate in one go" problem. Maybe for all current embind users this will not be a problem in practice(?) - if it is used only by smaller authors that control all their JS library code, then it is probably not a problem. (We don't use embind at Unity so we don't care about this)

We could create a function

function wbindFlexible64BitCallingConvention(sig, funcPtr, args) {
#if WASM_BIGINT
  return wbindArray(funcPtr)(args);
#else
  return dynCall(sig, funcPtr, args);
#endif
}

to wrap this to caller, but I don't see there being any sensible uses of this, so better to just explicitly ifdef the few places where it is a valid thing to do. (== case (1) above)

[sbc100]

I guess this is an example where building with WASM_BIGINT can generate functions with different signatures but in this case that is what we want since we always want the BigInt ABI to be used for invoke_xxx calls (since it a completely internal detail its OK that the signatures differ when switching to WASM_BIGINT)?

[juj]

Yes, I think this is the only one place where a ABI-agnostic "passthrough" is implemented, so we want to invoke the BigInt or dynCall version depending on WASM_BIGINT build mode.

I don't expect that user code would ever implement such a passthrough..

The only other place that does this currently is embind, and I think it is looking for trouble doing that right now. Either users will need to sprinkle ifdef WASM_BIGINTs all over their embind calling code, or they just wholesale migrate to WASM_BIGINT only code.

(Ideally I think embind should offer two different registration or calling mechanisms to register either BigInt ABI or i32 pairs ABI to allow breaking existing call sites when WASM_BIGINT mode is activated - though there is a chance that we'll never have users that will complain about this, if everyone is able to do such a wholesale-rewrite-all-embind-callsites migration - maybe 64-bit call sites will be very rare in practice(?))

[sbc100]

wbind is a library function .. so using it here in the a runtime function is tricky because we have no way to declare addFunctionWasm__deps on it. Maybe its time to move addFunctionWasm to a library function?

[sbc100]

Writing wbind here seems odd.. especially since its dual is setWasmTableEntry.

How about getWasmTableEntry to mirror setWasmTableEntry? Then getWasmTableEntry have have two different flavors and wbind can just be an alias for getWasmTableEntry in library.js?

If you don't like that idea how about the other way around.. make $getWasmTableEntry and alias of wbind?

[juj]

Maybe its time to move addFunctionWasm to a library function?

We have used this mechanism of "runtime depends on library and auto-adds the needed library funcs as always exported" in a number of places, so I'll go with that for now.

Refactoring addFunctionWasm to a library function does sound ok, but I'll keep that off from this code to not creep the PR size.

[juj]

Writing wbind here seems odd.. especially since its dual is setWasmTableEntry.

Here the function $setWasmTableEntry is strictly an internal JS library function, we don't want public users to ever use that. The function $wbind is a public JS function. Though we don't have a good way of differentiating these two. If I rename $wbind to $getWasmTableEntry, then it would look odd to have $setWasmTableEntry be an internal impl detail but $getWasmTableEntry be public.

Btw, @kripken why do we need to use a WeakMap for the functionsInTableMap code?

Also, why do we need to ensure that each function has a unique index? Like - if a caller is silly and calls addFunctionWasm several times for the same function, isn't that just their loss, and they'd be better off writing better add/remove semantics? Or is there some important pattern that necessarily results in redundant calls to addFunctionWasm several times for the same function?

[juj]

Hmm, there is some failure with respect to ASYNCIFY mode that I haven't been able to wrap my head around just yet.. @kripken do you know if there is some special consideration with respect to ASYNCIFY calling function pointers via dynCall()s or via wasmTable.get()? I see that some calls to non-64bit containing functions fail via wasmTable.get(), whereas they work via dynCall_ii() in ASYNCIFY mode.

[kripken]

do you know if there is some special consideration with respect to ASYNCIFY calling function pointers via dynCall()s or via wasmTable.get()?

There is one thing Asyncify does which I think no one else does, which is to remember how we called into the wasm module so that it can repeat that call (when we resume after pausing). For that reason ASYNCIFY enables DYNCALLS so that when we call indirectly into the wasm we in fact call an exported dynCall function - we never enter the module through a table call.

Not sure what could go wrong, though, if this PR does not stop DYNCALLS from being done when ASYNCIFY is? But, I am a little unsure what this PR does - does it just introduce a new API, or also use that API internally? (I assume it uses it, for it to help fix that performance issue? If so, then using it in fewer places could be a good way to debug to find out where things break with asyncify.)

[kripken]

(Btw, the change to use i32, i32 for i64s sounds good to me for the old API, for API stability, and other stuff mentioned here.)

[sbc100]

I propose that we split this into 3 changes (I'm happy to take case of (1) and (2) if you are busy).

1. Add caching, internal helper functions setWasmTableEntry/getWasmTableEntry (should speed up existing users of get()/set())
2. Move addFunctionWasm and friends from runtime functions to library functions.
3. Add wbind (the rest of the PR).

(1) and (2) are independent, and while (2) could easily happen as a followup I think it would be good to land (1) first so we can see the performance benefits independently of the ergonomics benefits of wbind.

[sbc100]

I propose that we split this into 3 changes (I'm happy to take case of (1) and (2) if you are busy).

1. Add caching, internal helper functions setWasmTableEntry/getWasmTableEntry (should speed up existing users of get()/set())
2. Move addFunctionWasm and friends from runtime functions to library functions.
3. Add wbind (the rest of the PR).

(1) and (2) are independent, and while (2) could easily happen as a followup I think it would be good to land (1) first so we can see the performance benefits independently of the ergonomics benefits of wbind.

I create a PR to land (1) as an NFC optimization (in case its useful): #15286.

I removed the !RELOCATABLE restriction as that didn't seem to be needed. We don't (today) free table regions or shrink or re-use table slots even in relocatable mode.

I'm not bothered about splitting out (2) ahead of time so maybe the rest of this change can land on top of #15286.

[juj]

I propose that we split this

Ok, closing this as stale.