1285972 - wasm: Implement Start section

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Implement the Start section as described there: https://github.com/WebAssembly/design/blob/master/Modules.md#module-start-function

From what I understand, the Start section is located before the code section (as shown in https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#high-level-structure , https://github.com/v8/v8/blob/master/src/wasm/wasm-module.h#L40 or https://github.com/WebAssembly/spec/blob/master/ml-proto/host/encode.ml#L392 ).

Out of the blue, I wonder why it is rather not *after* the code section, when we already know the actual number of functions.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

WIP, passing the simple test case I've provided with the patch. Needs more testing.

The strategy here is the following:
- after decoding the start section, we store the function index in the ExportMap.startIndex;
- in DecodeCodeSection when we have access to the number of functions, we run the checks on the startFuncIndex, and then transform the function index into an export index (because that's what we need to call the function later). At this point, we've already decoded function exports, so we can avoid recreating a stub if it's not needed.
- there's one thing we can't know: was the start function exported or not? In the worst case, it was not exported and we create an unused JSFunction in CreateExportObject, but it is going to be GC'd later. (in the best case, it was exported and the JSFunction is then used).

Comment 2

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> In the worst case, it was not exported and we create an unused JSFunction in
> CreateExportObject, but it is going to be GC'd later.

That seems fine for now.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: start.patch

Implementation with tests.

By adding a boolean `isStartFunctionExported` next to the startFunctionIndex, we could also get rid of the spurious JSFunction allocation; or we could even use e.g. the 17th bit of startFunctionIndex as this boolean (because MaxExports < 2**17). Thoughts?

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8770168 [details] [diff] [review]
start.patch

Review of attachment 8770168 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, thanks!

::: js/src/asmjs/WasmCompile.cpp
@@ +1080,5 @@
>  
>      if (numFuncBodies != mg.numFuncSigs())
>          return Fail(d, "function body count does not match function signature count");
>  
> +    if (startFuncIndex != NO_START_FUNCTION) {

The start section goes after the function section, and all function signatures are available after the function section, so I think this logic can simply be in DecodeStartSection.

::: js/src/asmjs/WasmGenerator.cpp
@@ +683,5 @@
>  
>  bool
> +ModuleGenerator::setStartFunction(uint32_t funcIndex)
> +{
> +    FuncIndexMap::AddPtr p = funcIndexToExport_.lookupForAdd(funcIndex);

Could you add an assertion in declareExport() that the start function is not set (thereby asserting we don't miss one)?

::: js/src/asmjs/WasmModule.h
@@ +122,3 @@
>  struct ExportMap
>  {
> +  private:

How instead changing ExportMap to a 'class' (and don't forget to update the forward decl in WasmInstance.h, lest clang warn-as-error you).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/535a3e0e2903
Baldr: Implement wasm start section; r=luke

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: isexported.patch

This implements the idea in comment 3 of using an high bit (that can never be set for an export index) to store whether it's exported or not.

The export index is set on bits from 0 to 15, the isExported bit is bit 16 (that is the 17th). If we allow exportIndex <= MaxExports, then bit 16 would be set if exportIndex == MaxExports == 1 << 16; to prevent this, we allow exportIndex < MaxExports. Note we could still allow this and use bit 17 instead, but this is kinda weird because the masking in startFunctionExportIndex() would allow many more values that it should (e.g. 65537).

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/535a3e0e2903

Comment 8

[Luke Wagner [:luke]]

Honestly, I'm not really sure it's worth the trouble.  There's going to be a little flurry of garbage created anyways during module compilation (in particular, the whole Module becomes garbage after instantiation and IDB-storage).

Comment 9

[Luke Wagner [:luke]]

Actually, with Table.prototype.get() the same issue arises except on O(n) scale: for every potentially get()ed table element, we need a FuncExport, but only the actually-exported functions must eagerly get a JSFunction in Module::instantiate().  So I can solve make sure the solution in bug 1284155 includes/subsumes the start-function special case.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

OK, as you prefer.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Alon, with this checked in yesterday (should be in today's nightly), you can use the start section in wasm modules.