Restore the dynCall() and dynCall_sig() API into the build #13296

juj · 2021-01-21T14:26:44Z

Restore the dynCall() and dynCall_sig() API into the build when -s USE_LEGACY_DYNCALLS=1 is passed. Add support for dynCall() in MINIMAL_RUNTIME builds (that was not implemented before).

This makes -s USE_LEGACY_DYNCALLS=1 a user-facing option.

…E_LEGACY_DYNCALLS=1 is passed. Add support for dynCall() in MINIMAL_RUNTIME builds (that was not implemented before).

juj · 2021-01-21T16:11:57Z

One question I have is whether we should rename the setting USE_LEGACY_DYNCALLS to WASM_DYNCALLS?

sbc100 · 2021-01-21T19:00:27Z

Looks good to me. I agree we could rename this option. If we are going to pick a new name I would go with just DYNCALLS or GENERATE_DYNCALLS?

If its not too annoying would you mind splitting out the MINIMAL_RUNTIME part? Is there a reason you chose a different interface for MINIMAL_RUNTIME? Why not just do dynCall_xx = asm['dynCall_xx]` just like normal functions and just like the normal runtime?

Going forward is the plan to transition from binaryen-generated dynCalls to table-base JS dynCalls? Presumably we could keep the exact same API while making this transition? I hope we can eventually remove the dynCall generation from binaryen

juj · 2021-01-21T20:07:30Z

If we are going to pick a new name I would go with just DYNCALLS or GENERATE_DYNCALLS?

Renamed to DYNCALLS.

If its not too annoying would you mind splitting out the MINIMAL_RUNTIME part?

I would rather not, if that is ok. This PR is already quite small that I hope it is well reviewable, I would like to keep the wrangling simple, and I already started splitting up that previous PR.

Is there a reason you chose a different interface for MINIMAL_RUNTIME? Why not just do dynCall_xx = asm['dynCall_xx]` just like normal functions and just like the normal runtime?

There does not exist asm['dynCall_xx'] after symbol name minification takes place. Classic runtime uses the Module export object to lookup the signature, i.e.

Module['dynCall_v'] = dynCall_v = asm['k'];
Module['dynCall_ii'] = dynCall_ii = asm['l'];
...
function dynCall(sig, ...) {
  Module['dynCall_'+sig].apply(null, ...);
}

whereas with MINIMAL_RUNTIME we do not have the Module object for exports, so it generates a dedicated dynCalls object with

var dynCall_v, dynCalls_ii, dynCalls = {};
...

dynCalls['v'] = dynCall_v = asm['k'];
dynCalls['ii'] = dynCall_ii = asm['l'];
...
function dynCall(sig, ...) {
  dynCalls[sig].apply(null, ...);
}

It does not make sense to use that structure for classic runtime, because it already has the Module object where the dynCalls are exported.

Going forward is the plan to transition from binaryen-generated dynCalls to table-base JS dynCalls?

Yes, that is the intent. When cached, table-based calls are faster than dynCalls (+61.7% faster for 'void' signatures, +81.2% for complex 'iffddii' signatures for static dispatch). We just need a good path to migrate forward from dynCalls to table-based calls. The issue here is that the change is global affecting everyone's code, so we need a way to let all JS code authors have a way to update at independent times before we drop dynCalls.

However I do notice this does have an effect that table-based lookups penalize Firefox performance on dynamic signatures (+27.7% slower vs dynamic dispatch via dynCall() on 'iffddii' signature), although even with that slowdown, Firefox is still more than three times faster than Chrome and Safari, so it's probably ok. Safari gets ~+18% faster, .

Presumably we could keep the exact same API while making this transition?

While we do the transition, I propose we keep the same API. But in general the dynCall() API is ill suited for table-based invocations, so after this lands, I'll make another PR about that new wbind() and wbindArray() API to replace dynCall().

sbc100

Great. I think I understand (and agree with) the direction now. Thanks for explaining.

src/parseTools.js

emscripten.py

src/library.js

src/settings.js

tests/test_core.py

kripken

The test here could be merged with test_dyncall_specific, I think? Adding to there would be shorter. lgtm with that.

kripken · 2021-01-26T20:50:25Z

tests/test_core.py

@@ -8270,6 +8270,19 @@ def test_gl_main_module(self):
    self.set_setting('MAIN_MODULE')
    self.do_runf(path_from_root('tests', 'core', 'test_gl_get_proc_address.c'))

+  def test_dyncalls(self):


Suggested change

def test_dyncalls(self):

@also_with_wasm_bigint

def test_dyncalls(self):

That will verify the emscripten.py changes here.

sbc100 · 2021-01-27T00:22:33Z

The current test failure in test-other is fixed on ToT so a merge/rebase should resolve it.

…Call_sig # Conflicts: # src/parseTools.js

sbc100 · 2021-01-27T16:51:31Z

tests/test_core.py

+      self.do_run_in_out_file_test('tests', 'core', 'test_dyncalls.c')
+
+      self.emcc_args += ['-s', 'MINIMAL_RUNTIME=1', '-s', 'DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=["$dynCall"]']
+      self.do_run_in_out_file_test('tests', 'core', 'test_dyncalls.c')


Sorry, I'm not sure sure what is going on with this test?

Why why where these lines added to this test rather than creating a new one? This test has a loop over 3 different modes but these 2 new do_run_in_out_file_test don't seem to use the loop variables at all.

Should the first part be it own test alongside test_dyncalls_minimal_runtime below? The second part looks identical to test_dyncalls_minimal_runtime.. unless I'm missing something?

Why why where these lines added to this test rather than creating a new one?

@kripken asked me in #13296 (review) to merge these two tests together. Good point about misplacing the tests inside the loop, I'll follow up with a PR to fix.

sbc100 · 2021-01-27T16:52:28Z

tests/core/test_dyncalls.js

+
+    // 2. Access a function pointer using the convenience/legacy 'dynCall' function (32-bit ABI)
+    //    (this form should never be used, it is suboptimal for performance, but provided for legacy compatibility)
+    var ret = dynCall('iii', funcPtr, [2, 3]); // Available only in WASM_BIGINT != 2 builds


Maybe remove the comments about WASM_BIGINT == 2 (which I don't think we planning to add in the end right?)

(Sorry for the late feedback).

Thanks, sharp eye, posted #13352 to fix.

* Follow up review on #13296. * Address review. Fix Asan on MINIMAL_RUNTIME. * Update test expectation * flake

sbc100 · 2024-03-22T17:18:33Z

Yes, that is the intent. When cached, table-based calls are faster than dynCalls (+61.7% faster for 'void' signatures, +81.2% for complex 'iffddii' signatures for static dispatch). We just need a good path to migrate forward from dynCalls to table-based calls. The issue here is that the change is global affecting everyone's code, so we need a way to let all JS code authors have a way to update at independent times before we drop dynCalls.

However I do notice this does have an effect that table-based lookups penalize Firefox performance on dynamic signatures (+27.7% slower vs dynamic dispatch via dynCall() on 'iffddii' signature), although even with that slowdown, Firefox is still more than three times faster than Chrome and Safari, so it's probably ok. Safari gets ~+18% faster, .

@juj, I was hoping to look into removal the underlying dynCall stuff in favor of just using the wasm table.

Can you share some info about the kind of benchmark you wrote for this? Was it just a simple microbenchmark calling and empty function in a loop, or something more complex?

sbc100 · 2024-03-22T17:20:19Z

Yes, that is the intent. When cached, table-based calls are faster than dynCalls (+61.7% faster for 'void' signatures, +81.2% for complex 'iffddii' signatures for static dispatch). We just need a good path to migrate forward from dynCalls to table-based calls. The issue here is that the change is global affecting everyone's code, so we need a way to let all JS code authors have a way to update at independent times before we drop dynCalls.
However I do notice this does have an effect that table-based lookups penalize Firefox performance on dynamic signatures (+27.7% slower vs dynamic dispatch via dynCall() on 'iffddii' signature), although even with that slowdown, Firefox is still more than three times faster than Chrome and Safari, so it's probably ok. Safari gets ~+18% faster, .

@juj, I was hoping to look into removal the underlying dynCall stuff in favor of just using the wasm table.

Can you share some info about the kind of benchmark you wrote for this? Was it just a simple microbenchmark calling and empty function in a loop, or something more complex?

Also, you might have mentioned this somewhere already, but do you have users/codebases that depend heavily on the performance of dynCalls (i.e. making many dyncalls per frame?).

juj · 2024-03-26T11:47:01Z

@juj, I was hoping to look into removal the underlying dynCall stuff in favor of just using the wasm table.

Unfortunately at Unity we are not ready for this. If we'd remove dynCall support in Emscripten, we would then need to have some kind of emulation layer adopted in Unity codebase. (though this would be a bit tricky to do, since the knowledge of possible signatures come at build time)

The challenge we have at Unity is that the Module.dynCall() and dynCall_xx() APIs are in the public layer, and Unity asset store web plugin authors have been utilizing these in their shipped commercial plugins. So we cannot break their code lightly.

We have shipped the new table based mechanism API, and have prepared documentation for this change. These are in beta channels currently.

In this area btw, I find that we don't quite have documentation for makeDynCall or getWasmTableEntry in Emscripten, so the "good" APIs are currently not as discoverable as they could. I think this is something we should look to improve.

Can you share some info about the kind of benchmark you wrote for this?

Looks like the benchmark was here: #12733 (comment)

juj added 2 commits January 21, 2021 16:23

Restore the dynCall() and dynCall_sig() API into the build when -s US…

d2da0bd

…E_LEGACY_DYNCALLS=1 is passed. Add support for dynCall() in MINIMAL_RUNTIME builds (that was not implemented before).

Add ChangeLog entry.

9bcac97

Rename USE_LEGACY_DYNCALLS to DYNCALLS.

69d8500

sbc100 reviewed Jan 21, 2021

View reviewed changes

src/parseTools.js Outdated Show resolved Hide resolved

sbc100 reviewed Jan 21, 2021

View reviewed changes

src/parseTools.js Outdated Show resolved Hide resolved

Inline getDynCaller.

02b40e8

sbc100 approved these changes Jan 22, 2021

View reviewed changes

emscripten.py Outdated Show resolved Hide resolved

src/library.js Outdated Show resolved Hide resolved

src/settings.js Show resolved Hide resolved

tests/test_core.py Show resolved Hide resolved

Address review.

5093390

kripken approved these changes Jan 26, 2021

View reviewed changes

juj added 3 commits January 27, 2021 13:20

Merge remote-tracking branch 'remotes/origin/master' into restore_dyn…

0f3e575

…Call_sig # Conflicts: # src/parseTools.js

Merge new dynCall test with old.

276373f

Reformat parseTools.js

08e843a

sbc100 approved these changes Jan 27, 2021

View reviewed changes

juj merged commit 7ff719c into emscripten-core:master Jan 27, 2021

sbc100 reviewed Jan 27, 2021

View reviewed changes

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

Follow up review on emscripten-core#13296.

12d25dc

juj mentioned this pull request Feb 4, 2021

Follow up review on #13296. #13352

Merged

juj added a commit to juj/emscripten that referenced this pull request Feb 8, 2021

Follow up review on emscripten-core#13296.

1445e1b

juj added a commit to juj/emscripten that referenced this pull request Feb 11, 2021

Follow up review on emscripten-core#13296.

664c8e6

juj added a commit to juj/emscripten that referenced this pull request Feb 13, 2021

Follow up review on emscripten-core#13296.

973bf21

juj added a commit that referenced this pull request Feb 13, 2021

Follow up review on #13296. (#13352)

d750311

* Follow up review on #13296. * Address review. Fix Asan on MINIMAL_RUNTIME. * Update test expectation * flake

sbc100 mentioned this pull request Mar 25, 2024

Calling wasm function pointers from JS regressions #12733

Closed

	def test_dyncalls(self):
	@also_with_wasm_bigint
	def test_dyncalls(self):

Restore the dynCall() and dynCall_sig() API into the build #13296

Restore the dynCall() and dynCall_sig() API into the build #13296

Uh oh!

Conversation

juj commented Jan 21, 2021

Uh oh!

juj commented Jan 21, 2021

Uh oh!

sbc100 commented Jan 21, 2021

Uh oh!

juj commented Jan 21, 2021

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

kripken Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Jan 27, 2021

Uh oh!

sbc100 Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

juj Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

juj Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Mar 22, 2024

Uh oh!

sbc100 commented Mar 22, 2024

Uh oh!

juj commented Mar 26, 2024

Uh oh!

Uh oh!