Fix regression that broke calls to makeDynCall in JS library code.

[juj]

Fix regression that broke calls to makeDynCall in JS library code. Old syntax is {{{ makeDynCall('sig') }}}(funcPtr, arg1, arg2);. New syntax is {{{ makeDynCall('sig', 'funcPtr') }}}(arg1, arg2). With this change, both old and new syntax work, with a compile time warning issued for old syntax to migrate to new one. Add ChangeLog entry about broken static dynCall invocation outside JS library files.

[kripken]

I love the clear error message, but actually handling the old form feels excessive to me. I suggest erroring the build, to force people to update? It's an internal API, and one that isn't hard to update for (with the nice new error message here).

[juj]

I am surprised to see you say that this is an internal API.

If this was an internal API (only used in Emscripten system library_*.js files), then indeed this would be excessive and I would not have minded too much about this, but would just fix our own library JS code in Unity and move on. However, {{{ makeDynCall(..) }}} works and is used also in user-provided library_*.js files, which makes this a public API.

Also we should not have core syntax in library_*.js that is not supposed to be used by end users. That is because most of the library_*.js files that people author are monkeyed off of existing src/library_*.js files - that is what we write to the GitHub issue tracker so many times in the past when people ask how to achieve something ("take a look at an example in src/library_webgl.js" etc). If {{{ makeDynCall(..) }}} was an internal API, it should certainly have been blocked from functioning in user library_*.js files at all. But why should user library_*.js files be somehow tiered to a lesser feature set than our own core library_*.js files?

Historically, according to my understanding, there have been three ways to invoke a function pointer from JS code:

1. dynCall_sig(): static dispatch, works from library_*.js files and EM_ASM, EM_JS, --pre-js-, --post-js and external <script>s.
2. {{{ makeDynCall('sig) }}}: static dispatch, works from library_*.js files only. The "safe" and preferred way to do static dispatch since it works across all build modes (EMULATED_FUNCTION_POINTERS, USE_LEGACY_DYNCALLS).
3. dynCall('sig'): dynamic dispatch, works from library_*.js files and EM_ASM, EM_JS, --pre-js-, --post-js and external <script>s. Carries a significant performance hit compared to static dispatch.

PR #12059 regressed all of the syntaxes 1., 2. and 3. above (#12733).

This PR fixes the regression (2).

I do not know how to fix (1) at the moment, that is blocking us to be able to update Unity to a newer compiler version.

The regression of (3) is that dynCall was removed from the build by default (which I agree is nice for code size), requiring library_*.js authors to now explicitly add a __deps field to it for it to work, or add it to DEFAULT_LIBRARY_FUNCS_TO_INCLUDE explicitly, if one wishes to do dynCall() from EM_ASM, EM_JS, --pre-js-, --post-js or external <script>s.

This regression (3) is minor for Unity's use, since we can paper over that in the build system easily, so not sweating too much about it (but it is still a breaking change that was not mentioned in the ChangeLog for Emscripten 2.0.2).

Chatting with @sbc100 on #12733 I believe there was a misassumption that (3) would be the "one syntax to rule them all". Historically I moved us away from the dynamic dispatch dynCall in Jan 2017 because of its large impact observed on real-world performance (#4894). The benchmark in #12733 (comment) verifies that the performance landscape is still the same today. We should not be going back to that.

Unity Asset Store has hundreds of packages that advertise WebGL support, and we have thousands to tens of thousands of users that develop their own WebGL project; of all of those an unknown number of packages and game projects WebGL packages with library_*.js files to interop with Unity C#, C/C++ and JS. We cannot break any of them when updating, because it may not even be up to the project developer to be able to fix up the library_*.js issue, but they must receive an update from the Asset Store package author.

If we errored out here in this message, that would mean that possibly thousands of users would be blocked from developing their game project until plugin authors provided a fix. That is not possible for us.

Because of these reasons is why I am adding a very explicit warning about this that will prompt developers who see the warning to look at their own library_*.js files, and nudge their plugin vendors to fix up theirs.

See also the comment #12733 (comment).

[kripken]

I see, thanks @juj, I didn't realize this was used in user code so much. I'd assumed that low-level handling indirect calls is extremely rare (like it is in the emscripten JS libraries). Optimally user code would use something that does the low-level handling for it, but that's not the situation.

Given that, sounds good to support the old syntax.

[sbc100]

Side note: I think there is a wider discussion to be had we might choose to make certain parts of our library code internal. Right now even because of the way libraries work everything is effectively public, but it would be nice to have a way to mark internal APIs as such so that we are not locked into them.

Regarding this specific case, it does seems that is kind of de-facto public API: I don't think (2) and (3) will be hard to fix. This change fixes (2) and, as you say, (3) is basically about opting into that library function. If (3) proves to be a big issue we can always add it to the default list of library funcs to include.

So that just leaves (1) which is the direct usage of old dynCall_xxx functions which used to be generated by wasm-emscripten-finalize but are no longer. In the old code there was always exactly one per signature in the table. It seems sad to conservatively generate all of those in JS even if they are not needed. Perhaps we can somehow generate just the needed ones by scanning the generated JS code?

FWIW, the result of {{{ makeDynCall }}} should be even more efficient that the direct call to dynCall_xxx since the table.get will end up inline.

[juj]

I think there is a wider discussion to be had we might choose to make certain parts of our library code internal. Right now even because of the way libraries work everything is effectively public, but it would be nice to have a way to mark internal APIs as such so that we are not locked into them.

Agree, though I think that set of internal API area is quite close to zero. I cannot think of many features that would need to be there for our internal use that we would not want to expose to external users, except for a few internal helpers.

But calling wasm function pointers from JS code is a core interop/language feature that certainly is in the public namespace.

In the old code there was always exactly one per signature in the table. It seems sad to conservatively generate all of those in JS even if they are not needed. Perhaps we can somehow generate just the needed ones by scanning the generated JS code?

My understanding was that we generated exactly one dynamic dynCall_sig for each function pointer signature that C/C++ code took an address of? Also my understanding was that Closure would be able to clean up the ones that are not actually called by the project's JS code.

Agree that it is unfortunate to generate all of them, and then need to rely on Closure to clean them up. That is something I have argued against on a lot of things (we should not generate something just to require Closure to pick up the mess afterwards, since not everyone can use Closure). It would be great if those can be somehow detected on what actually need to be emitted. But that mechanism will need to allow injecting external dependencies into, since any analysis code we write will not be able to see external JS code in outside <script>s that may invoke dynCall_sigs.

FWIW, the result of {{{ makeDynCall }}} should be even more efficient that the direct call to dynCall_xxx since the table.get will end up inline.

Like the above benchmarking shows, calling the result of {{{ makeDynCall }}} is indeed more efficient than a direct call to dynCall_sig (when one can cache the result of {{{ makeDynCall }}} somewhere), but {{{ makeDynCall('sig', 'funcPtr' }}}(arg1,arg2); is slower than dynCall_sig(funcPtr, arg1, arg2);.

[sbc100]

Like the above benchmarking shows, calling the result of {{{ makeDynCall }}} is indeed more efficient than a direct call to dynCall_sig (when one can cache the result of {{{ makeDynCall }}} somewhere), but {{{ makeDynCall('sig', 'funcPtr' }}}(arg1,arg2); is slower than dynCall_sig(funcPtr, arg1, arg2);.

Hopefully that will no longer be true once #12741 lands.
makeDynCall should resolve to just table.get(i)(arg, arg) which I would hope would be that same performance as the old wasm-side dynCall_sig functions which just basically do exactly that. My ideas was to generate dynCalls like this in JS:

function dynCall_iii(f, arg1, arg2) {
   return table.get(f)(arg1, arg2);
}

I'm hoping that will be as performant as the old wasm-side dyncalls. Even if its not quite there it should be super close.

[sbc100]

In case you missed the original motivation for removing the automatic generation of dynCalls in wasm-emscripten-finalize, its part of our goal to avoid doing any work in wasm-emscripten-finalize for debug builds. This is important for large debug builds where wasm-emscripten-finalize is super slow and can run out of memory dealing with re-writing the DWARF information.

This change, combined with a lot of other work, allow emscripten to directly consume and run the output of wasm-ld without wasm-emscripten-finalize having to mess with the wasm binary.

[kripken]

lgtm on the code, but I have not yet read the details for the plans for addressing the remaining cases (but I assume this is alignment with any such plans).

[juj]

In case you missed the original motivation for removing the automatic generation of dynCalls in wasm-emscripten-finalize, its part of our goal to avoid doing any work in wasm-emscripten-finalize for debug builds. This is important for large debug builds where wasm-emscripten-finalize is super slow and can run out of memory dealing with re-writing the DWARF information.

Thanks, yeah, that rationale does sound good on its own.