Part two of dynCall removal #12059

sbc100 · 2020-08-26T22:38:17Z

This change now passes flags to binaryen to limit the creation
of the dynCall functions and removes more internal use of the
legacy dynCall functions.

See #12002

emcc.py

tools/shared.py

tests/return64bit/testbind.js

kripken · 2020-08-26T22:48:55Z

tests/code_size/hello_webgl_wasm.json

-  "a.wasm.gz": 6935,
-  "total": 16029,
-  "total_gz": 9552
+  "a.js": 4651,


what causes the increase here?

I think this is the JS side code in dynCall and makeDynCaller. The wasm gets smaller in proportion to the number of signatures and the JS grows by a fixed amount I think.

In summary:

Small JS size increase.

Only paid by programs that use dynCalls.

Should be removable once we can switch 100% to table calls.

-1 for this change: the old code is both smaller and more performance, as it avoids dynamic signature binding. I think we should switch to this only after we have step 3. from above in-place in this same PR. That way we will not have to accept a regression on a promise of things improving back to old form later on.

OK that makes sense. I'll keep going with this until its all done rather then try to land this incremental change.

kripken · 2020-08-26T22:53:22Z

src/library_async.js

@@ -340,6 +340,7 @@ mergeInto(LibraryManager.library, {
    });
  },

+  emscripten_scan_registers__deps: ['$getDynCaller'],


Why is this dep added? I don't see the function used in the body. I'd guess it's expanded from {{{ makeDynCall }}}? It seems confusing to not see the function in the dep actually used, and could make us forget deps later on. Could the one in the body be changed?

Yes, any function with a makeDynCall need to depend on getDynCaller for now.

getDynCaller then selects between the legacy native thunks and the direct table access. Once we can remove the legacy thunks completely we can look at making this more efficient.

juj · 2020-08-27T12:45:11Z

src/library_html5.js

@@ -2656,6 +2656,7 @@ var LibraryJSEvents = {
    cancelAnimationFrame(id);
  },

+  emscripten_request_animation_frame_loop__deps: ['$getDynCaller'],


This kind of deps mechanism is a bit awkward since it leaks the need to know internal names from makeDynCall out to the JS code. Can we align the naming so that one would type the same string into the __deps field that one puts inside the {{{ }}} block. That way there is at least a visual hint to suggest where the dependency comes from. (otherwise it can be hard to discover)

I manged to remove the need for these.

sbc100 · 2020-08-27T15:54:42Z

OK I went all the way to managed to get a codeside win for both wasm and js sides!

We still have three different cases where we need to use the legacy system:

WASM2C - needs to be modified not to rely on the native dynCall_xx functions
ASYNCIFY - seems to depend on the dynCall entry points being asyncified whereas perhaps the table entries are not? Is not clear to me yet.

Both of these cases needs to be investigated and fixed so that we can completely remove the non-i64 dynCalls from binaryen.

kripken

Nice progress!

emcc.py

src/library.js

kripken · 2020-08-27T16:29:12Z

src/library.js

+    }
+    return Module['dynCall_' + sig].call(null, ptr);
+  },
+  $dynCall__deps: ['$dynCallLegacy'],


it's odd to have the deps not near the function - why is this?

Because the dependency is conditional in the exact same way the this functions existence is conditions.

I guess I don't feel strongly but I believe this is the one place in the entire codebase not to have the two adjacent. Maybe it's worth the benefit though.

src/library_exceptions.js

src/library_websocket.js

src/parseTools.js

src/worker.js

tests/return64bit/testbind.js

kripken · 2020-08-27T16:35:57Z

And good idea about the internal API change, yes, that's a bunch of work to update all the places as you've done, but I think it's the right thing.

sbc100 · 2020-08-27T17:14:53Z

And good idea about the internal API change, yes, that's a bunch of work to update all the places as you've done, but I think it's the right thing.

I manged to make vim do it for me after doing about 10 by hand: :%s/makeDynCall('$.*$') }}}($[^,)]*$, /makeDynCall('\1', '\2') }}}(/c

sbc100 · 2020-08-27T17:40:17Z

Add two bugs for followup on ASYNCIFY and WASM2C

Instead, this calls the Table directly. This takes a little more effort at compile time because we need to find the index of the function type for the signature we are calling with (so that we trap if the target function has the wrong type). To do that, scan the wasm2c output - which is at risk of breaking if that output changes, sadly (another option might be to disassemble the wasm and scan that, which would at least be a stable format - but this seems fine for now). Helps #12059 Fixes #12065

kripken

lgtm % last comment on always including $dynCall.

kripken · 2020-08-28T23:58:58Z

tools/ports/sdl2.py

@@ -90,7 +90,7 @@ def clear(ports, settings, shared):


 def process_dependencies(settings):
-  settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += ['$autoResumeAudioContext']
+  settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += ['$autoResumeAudioContext', '$dynCall']


I think it would be safer to always do this for dynCall in all (non-minimal runtime) builds - that is, to do this in emcc.py. That would make this change less noticeable to users.

Separately, removing it from DEFAULT_LIBRARY_FUNCS_TO_INCLUDE later might make sense, but it would be good at that later time to document that in the changelog + make sure it gives a clear error to users if they need to add it manually from then onwards.

kripken · 2020-08-29T20:28:15Z

I did some benchmarking of this and see no issues, in fact it may be a slight bit faster.

This change now passes flags to binaryen to limit the creation of the dynCall functions and removes more internal use of the legacy dynCall functions. See #12002

This removes callStackIdToFunc, the map of ids to functions. Instead it makes us always use the map of ids to function names. That removes one of the two mechanisms and makes the code simpler, which will help in a followup PR that adds support for direct table calls, which we need for #12059. I think we kept both mechanisms because I was worried about performance, but I think I was wrong. After debugging for #12059 I think I have a better understanding of things and it seems fine to use names - it's just a single JS map lookup for asyncify operation, which is small compared to the actual asyncify overhead. Move some code to maybeStopUnwind, with no changes. That code will be useful in the followup as well. Also clean up some debugging code to make it more useful.

… issue emscripten-core#12733.

sbc100 requested a review from kripken August 26, 2020 22:38

sbc100 force-pushed the dyn_call_part2 branch from 82a998c to 88edaed Compare August 26, 2020 22:40

kripken reviewed Aug 26, 2020

View reviewed changes

juj reviewed Aug 27, 2020

View reviewed changes

sbc100 force-pushed the dyn_call_part2 branch 5 times, most recently from 5a7eb1e to 122c607 Compare August 27, 2020 15:22

kripken reviewed Aug 27, 2020

View reviewed changes

This was referenced Aug 27, 2020

wasm2c is hardcoded to depend on dynCall_xx exports from native code #12065

Closed

test_fibers_asyncify fails without the binaryen generated dynCall functions. #12066

Open

kripken mentioned this pull request Aug 27, 2020

Stop using dynCalls in wasm2c #12070

Merged

sbc100 force-pushed the dyn_call_part2 branch 2 times, most recently from 0ccdcbc to c60a031 Compare August 28, 2020 08:53

kripken approved these changes Aug 29, 2020

View reviewed changes

kripken mentioned this pull request Aug 29, 2020

Refactor and simplify asyncify logic for calling back in #12081

Merged

kripken mentioned this pull request Aug 31, 2020

Handle entering the wasm via a table call in Asyncify #12088

Closed

sbc100 added 2 commits August 31, 2020 10:03

Part two of dynCall removal

dbf71f5

This change now passes flags to binaryen to limit the creation of the dynCall functions and removes more internal use of the legacy dynCall functions. See #12002

feedback

23b4067

sbc100 force-pushed the dyn_call_part2 branch from c60a031 to 23b4067 Compare August 31, 2020 17:06

.

19fc475

sbc100 merged commit 953aded into master Aug 31, 2020

sbc100 deleted the dyn_call_part2 branch August 31, 2020 18:07

juj mentioned this pull request Nov 8, 2020

Calling wasm function pointers from JS regressions #12733

Closed

sbc100 mentioned this pull request Nov 9, 2020

Optimize makeDynCall to avoid dynamic dynCall where possible #12741

Merged

juj mentioned this pull request Nov 10, 2020

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

Merged

juj added a commit to juj/emscripten that referenced this pull request Jan 20, 2021

Fix dynCall API after regression from PR emscripten-core#12059. Fixes…

3563c51

… issue emscripten-core#12733.

juj mentioned this pull request Jan 20, 2021

RFC: Fix dyncalls api #13289

Closed

juj added a commit to juj/emscripten that referenced this pull request Jan 20, 2021

Fix dynCall API after regression from PR emscripten-core#12059. Fixes…

df01a0d

… issue emscripten-core#12733.

juj mentioned this pull request Oct 28, 2021

Add JS-side cache of wasm table entries. NFC #15286

Merged

juj mentioned this pull request Mar 13, 2025

Fix non-future-proof invocations of Emscripten dynCalls thirdweb-dev/unity#232

Closed

Part two of dynCall removal #12059

Part two of dynCall removal #12059

Uh oh!

Conversation

sbc100 commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Aug 27, 2020

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken commented Aug 27, 2020

Uh oh!

sbc100 commented Aug 27, 2020

Uh oh!

sbc100 commented Aug 27, 2020

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Aug 29, 2020

Uh oh!

Uh oh!

sbc100 commented Aug 26, 2020 •

edited

Loading