Ensure function table indexes are unique #10741

kripken · 2020-03-20T23:40:22Z

This ensures that our table contains unique functions, at least
as far as addFunction is concerned: it checks if there is already
an index for the function, and if so, returns that index.

This is good for avoiding wasted table indexes, and also is part
of a solution for more efficient MAIN_MODULE function pointer
index handling, which I will open in Binaryen. Basically, that PR
will just let main modules add their own functions to the table,
and not call any fp$ accessors; the risk is that while doing
so we need to tell the loader to use those same indexes for
other modules that want function pointers to the same functions.
By always having unique indexes in the table thanks to this PR
we achieve that without any special effort.

sbc100

Maybe make separate function for this behavior? getAddressForFunction?

sbc100 · 2020-03-21T00:10:18Z

src/runtime_functions.js

+  }
+  if (functionsInTableMap[func]) {
+    return functionsInTableMap[func];
+  }


One downside of doing this is that it will mask the current bug we are trying to investigate with @awtcode...

If this fixes a bug, then it's a proper fix for that bug, isn't it? That is, this isn't doing a random thing that might fix a function pointer comparison - it is literally making function pointers properly unique, which should fix bugs.

Maybe I don't see what you mean here?

The problem is that the module in which the bug occurs seem to be confused about how to find the address of a certain function.

For internal (dll-local) functions the compiler just puts the function in the elem section and should never call the fp$ function. For external (non-dll-local) function the compiler should not put the function in the elem section and only call the fp$ function.

This fix will allow such confused modules to silently work.

Hmm, I don't see that as silent. I think this PR changes the model entirely to what should have been the proper model all along: the Table has unique elements. That's more efficient and guaranteed to be correct.

As a bonus, it means we don't need to worry about coordinating things like different ways to add things to the Table. You're right that this might fix a bug in coordinating such things, but it does so by allowing us to remove that code! That's essentially what the corresponding binaryen PR does.

sbc100 · 2020-03-21T00:12:25Z

src/runtime_functions.js

 var freeTableIndexes = [];

+#if WASM_BACKEND
+// Weak map of functions in the table to their indexes, created on first use.


Why wasm backend only? if this behaviour change is going to be made why of make it apply to fastcomp too?

Well, I only really understand how function pointers work in upstream, after spending the day on that. So this might work on fastcomp, but it's risky - as it's deprecated I figured safest to avoid any changes?

But all this change is going is duplicating the results of adding functions to the wasm table. Surly this will trivially work in both backends and if the test passes.. why not just remove the ifdef. At the very least there is a readability cost to adding more ifdefs.

If you think the risk is worth it I can remove the ifdef, sure.

kripken · 2020-03-21T14:48:08Z

I did some benchmarking to validate that this actually helps (together with the binaryen PR), and doesn't hit some weird slow case in scanning the Table from JS, or in WeakMap handling. On hello_libcxx.cpp (hello world that ends up linking in libc++) with MAIN_MODULE we have almost 1,000 items in the table, so it's not a trivial testcase:

Runnng it is 40% faster overall, almost all of which is startup time (since it just prints "hello world").
Specifically the ___assign_got_enties() call goes from 60ms to 12ms, a 5X speedup.
The new work added here, which is scanning the Table from JS to build the WeakMap, takes 1-2ms total.
The JS is 20% smaller (due to removing fp$ stubs - maybe we could have optimized them away in another way though, they do seem excessively large atm).
The wasm is 3% smaller (due to removing imports and calls).

awtcode · 2020-03-21T15:57:08Z

Thanks for doing this @kripken :) I can't wait to try it out :)

sbc100 · 2020-03-21T16:26:43Z

tests/interop/test_add_function.cpp

+    assert(wasmTable.length >= 3);
+    assert(addFunction(wasmTable.get(1)) === 1);
+    assert(addFunction(wasmTable.get(2)) === 2);
+  }, &foo, &bar, &baz);


Are foo, baz, bar even used here ?

They are not used inside the JS, but sending their addresses into JS ensures that their addresses are taken (in a way LLVM can't optimize out) and therefore that the Table is at least of that size.

Can we do that more explicitly?

Or at least add a comment.

sbc100

Lgtm with comment

This reverts commit e620ae9.

Depends on emscripten-core/emscripten#10741 which ensures that table indexes are unique. With that guarantee, a main module can just add its function pointers into the table, and use them based on that index. The loader will then see them in the table and then give other modules the identical function pointer for a function, ensuring function pointer equality. This avoids calling fp$ functions during startup for the main module's own functions (which are slow). We do still call fp$s of things we import from outside, as we don't have anything to put in the table for them, we depend on the loader for that. I suspect this can also be done with SIDE_MODULES, but did not want to try too much at once.

… pointer indexes in fastcomp because we are doing a matching the on the stubs instead of the wasm function wrapper. In this change, we get the actual function wrapper and pass it into addFunction() so that addFunction can use this to compare with the function wrapper gotten from the wasm table.

kripken added 6 commits March 20, 2020 13:18

wip [ci skip]

666972c

clarify [ci skip]

0251179

fix [ci skip]

de67dc8

proper fix

cbb815d

testing

156ae8f

finish

590466c

kripken requested a review from sbc100 March 20, 2020 23:40

fix weak map code

d37dc54

sbc100 reviewed Mar 21, 2020

View reviewed changes

kripken mentioned this pull request Mar 21, 2020

Avoid fp$ access in MAIN_MODULES WebAssembly/binaryen#2704

Merged

kripken added 3 commits March 21, 2020 08:44

unifdef

6556027

Merge remote-tracking branch 'origin/master' into tab

2078eba

fix test for fastcomp

e620ae9

sbc100 reviewed Mar 21, 2020

View reviewed changes

sbc100 approved these changes Mar 21, 2020

View reviewed changes

kripken added 2 commits March 21, 2020 09:37

Revert "fix test for fastcomp"

7536ebb

This reverts commit e620ae9.

review feedback: add a comment

5b5cb90

kripken merged commit c64007e into master Mar 21, 2020

kripken deleted the tab branch March 21, 2020 21:46

kripken mentioned this pull request Mar 23, 2020

Duplicate function address of an exported function #10693

Closed

Mintyboi mentioned this pull request Apr 13, 2020

Fix fp accessor for fastcomp #10895

Closed

Mintyboi mentioned this pull request Apr 13, 2020

Ensure unique function table indexes for side modules #10899

Closed

Mintyboi mentioned this pull request Apr 13, 2020

Update creation of function pointer in fastcomp #10900

Closed

Ensure function table indexes are unique #10741

Ensure function table indexes are unique #10741

Uh oh!

Conversation

kripken commented Mar 20, 2020

Uh oh!

sbc100 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

kripken commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awtcode commented Mar 21, 2020

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kripken commented Mar 21, 2020 •

edited

Loading