Avoid fp$ access in MAIN_MODULES #2704

kripken · 2020-03-21T00:15:46Z

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.

sbc100 · 2020-03-21T00:48:42Z

I wonder if there is some way we could do this wasm-ld directly?

sbc100 · 2020-03-21T00:51:01Z

Should we do that same for gotMemEntries for consistency?

kripken · 2020-03-21T14:53:40Z

I wonder if there is some way we could do this wasm-ld directly?

Good question, yeah, please see this PR as a proof-of-concept: it shows that if the Table entries are unique then we can just avoid the fp$ mechanism entirely. If we can do so in wasm-ld in a nicer way, that sounds better to me.

But it would mean that wasm-ld assumes the Table is kept unique by the dynamic loader.

Should we do that same for gotMemEntries for consistency?

That's not as easy, as this depends on the dynamic loader scanning the Table to avoid duplication. There isn't such a thing for memory entries. We could avoid g$ calls perhaps by adding exports to inform the dynamic loader of where things are, but that's not going to be as efficient as the Table approach here.

sbc100 · 2020-03-21T15:00:10Z

We already exports that address of all non-local symbols, but we export the addresses relative to __memory_base.

I think the following algorithm would work:

for rel in  relocations:
  if symbol.is_locally_defined():
    GOT.mem.symbol = symbol.base_offset + __memory_base

That way the g$ calls only remain for data symbols outside the current DLL.

awtcode · 2020-03-21T16:05:53Z

I wonder if there is some way we could do this wasm-ld directly?

Good question, yeah, please see this PR as a proof-of-concept: it shows that if the Table entries are unique then we can just avoid the fp$ mechanism entirely. If we can do so in wasm-ld in a nicer way, that sounds better to me.

But it would mean that wasm-ld assumes the Table is kept unique by the dynamic loader.

I was under the impression that wasm-ld should generate a unique Table in the first place? As in, if a function already exists in a Table, then wasm-ld should not generate a dynamic loader for it.

kripken · 2020-03-21T18:26:48Z

@sbc100 oh, if we export the relative global addresses already, then we can use that in the loader for globals, yes. It would require some work on the JS side too, but seems possible. (How important is that, btw?)

@awtcode I'm not sure if wasm-ld creates a unique Table or not by itself, but that does seem likely. But regardless what this approach would change is that a module would place its own items in the Table, and assume that the loader knows how to reuse those indices properly. That's a new contract between wasm-ld and the loader, compared to before: instead of the loader doing everything, wasm-ld is emitting code that needs to work with the loader.

sbc100 · 2020-03-21T18:45:15Z

@sbc100 oh, if we export the relative global addresses already, then we can use that in the loader for globals, yes. It would require some work on the JS side too, but seems possible. (How important is that, btw?)

I'm not sure I understand. What extra would would it require any work on the JS side? The JS already has to implement g$ functions which take the exported relative addresses and add the __memory_base. All I'm proposing is to internalize this here, to avoid calling the g$ function, just like you are doing for the fp$ functions. If important only for consistency, and because it would additionally improve startup time by removing those calls from assign_got_entries.

There is no need to make it part of this PR I guess, but I do think I think it should be doable, and without any JS-side changes.

@awtcode I'm not sure if wasm-ld creates a unique Table or not by itself, but that does seem likely. But regardless what this approach would change is that a module would place its own items in the Table, and assume that the loader knows how to reuse those indices properly. That's a new contract between wasm-ld and the loader, compared to before: instead of the loader doing everything, wasm-ld is emitting code that needs to work with the loader.

kripken · 2020-03-21T18:53:29Z

Oh, I see, the JS is already doing that? (I just read the function pointer code for this, not the globals code.) Sounds good then, yeah, should be just to internalize in binaryen as an optimization basically.

Yeah, let's keep that a separate PR.

sbc100 · 2020-03-21T19:17:15Z

src/ir/table-utils.h

    }
  }
 };



Should FlatTable go in this new namespace too?

Personally I think it's excessive to add it - it's name already implies what type of thing it does. But I don't feel strongly. What do you think?

Just seems odd to have a TableUtils that is declared in table-utils.h but not all the stuff in that header ends up in the namespace. No need to hold up this PR on that though. I don't feel super strongly about it.

Fair enough, if it seems odd to you, I'll change that.

sbc100 · 2020-03-21T19:19:12Z

src/wasm/wasm-emscripten.cpp

  wasm.addFunction(post_instantiate);

-  if (Function* F = generateAssignGOTEntriesFunction()) {
+  if (Function* F = generateAssignGOTEntriesFunction(true /*isSideModule*/)) {


The chromium/llvm style for this is /*isSideModule=*/true (with no spaces I think). Not sure if this is a common pattern in binaryen?

Looking around, it seems like we mostly use bools with .setX() functions. I'll adapt to that.

sbc100 · 2020-03-21T19:23:34Z

src/wasm/wasm-emscripten.cpp

+          ExpressionManipulator::copy(wasm.table.segments[0].offset, wasm);
+        auto* add = builder.makeBinary(AddInt32, getBase, c);
+        auto* globalSet = builder.makeGlobalSet(g->name, add);
+        block->list.push_back(globalSet);


I think we may have forgotten to take into account __table_base here, and also the tablesize in the dylink section?

If we create the table during this pass we will most likely also need to create __table_base which would otherwise have been created by lld.

See the ::copy operation: we are copying the base of the segment, and adding that. The tests show we handle the case of __table_base as well as if it's a constant.

If we create the table here, then we create it with a constant offset. I think that won't happen in dynamic linking since we will always import the table there, though?

About dylink, yeah, thinking on that now, it seems like we need to update it if it exists. Looks like we have no handling of that at all in binaryen yet.

What do you think about doing this in binaryen vs wasm-ld? After your comments here it is starting to seem to me like wasm-ld might be simpler (but I don't know the code there).

I thought about it and I think doing in lld would require more re-design. I'm OK with landing this change first/now, while I look at perhaps trying do this in wasm-ld.

Sounds good to do it here.

Meanwhile I looked into the dylink section issue. It would require first adding full parsing and writing support for that section, and for updating it through operations, so basically adding IR support for it. If we want that we should do it separately from this PR I think. And that doesn't need to block this PR as this one only works on main modules where there is no dylink section.

The MAIN_MODULE does have a dylink section. We should probably fix binaryen to preserve that section by I agree that is a separate PR.

Oh interesting, that's a difference in upstream compared to fastcomp that I didn't realize.

Emscripten never uses it in a main module, though, I verified.

sbc100 · 2020-03-27T18:09:07Z

src/ir/table-utils.h

    }
  }
 };



Just seems odd to have a TableUtils that is declared in table-utils.h but not all the stuff in that header ends up in the namespace. No need to hold up this PR on that though. I don't feel super strongly about it.

sbc100 · 2020-03-27T18:10:50Z

src/wasm/wasm-emscripten.cpp

+        // We may be able to do something for side modules as well, however,
+        // that would require at least updating the dylink section.
+        if (f->imported()) {
+          Fatal() << "GOT.func entry export but not implemented: " << g->base;


This error could be better maybe? if you hit this does it mean that GOT.func.foo is both imported and exported?

Yes, that's equivalent. I'll rephrase the error that way.

WebAssembly/binaryen#2704 will require some minor updates to the main module test, as we will be optimizing out some fp$ things.

kripken added 15 commits March 20, 2020 11:49

wip [ci skip]

1412b2e

builds [ci skip]

e7e104b

fix

b326d60

format [ci skip]

288534f

nicer [ci skip]

ef8d0d4

fix [ci skip]

c7d575e

style [ci skip]

9d8f614

fix [ci skip]

b17779d

we don't need this

383a7dd

we don't need this either

f5b6e9d

cleanup

4d1bbad

finish

36136b7

better

23dd934

tests

a76c620

style

8e701db

kripken requested a review from sbc100 March 21, 2020 00:18

sbc100 reviewed Mar 21, 2020

View reviewed changes

kripken added 5 commits March 21, 2020 13:41

nicer pattern for sideModule bool

72a43fc

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

321fbca

style

c355924

fix build

a51faf8

comment

f36fd3d

fix

6fd21bd

kripken requested a review from sbc100 March 27, 2020 17:58

sbc100 approved these changes Mar 27, 2020

View reviewed changes

kripken added 5 commits March 27, 2020 15:23

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

4afe42a

FlatTable => TableUtils::FlatTable

c70bc3c

format

a058399

error clarification

d66e0f4

format

f82e6bc

kripken merged commit d595a45 into master Mar 28, 2020

kripken deleted the tab branch March 28, 2020 00:25

kripken mentioned this pull request Mar 28, 2020

Disable a metadce test to allow a binaryen roll emscripten-core/emscripten#10795

Merged

kripken added a commit to emscripten-core/emscripten that referenced this pull request Mar 28, 2020

Disable a metadce test to allow a binaryen roll (#10795)

66241df

WebAssembly/binaryen#2704 will require some minor updates to the main module test, as we will be optimizing out some fp$ things.

Avoid fp$ access in MAIN_MODULES #2704

Avoid fp$ access in MAIN_MODULES #2704

Uh oh!

Conversation

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

Uh oh!

Uh oh!

sbc100 commented Mar 21, 2020

Uh oh!

sbc100 commented Mar 21, 2020

Uh oh!

kripken commented Mar 21, 2020

Uh oh!

sbc100 commented Mar 21, 2020

Uh oh!

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

Uh oh!

Uh oh!

kripken commented Mar 21, 2020

Uh oh!

sbc100 commented Mar 21, 2020

Uh oh!

kripken 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!

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!

Choose a reason for hiding this comment

Uh oh!

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

awtcode commented Mar 21, 2020 •

edited

Loading