Closed Bug 1285972 Opened 6 years ago Closed 6 years ago

wasm: Implement Start section

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox50 --- affected

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: wasm
Attached patch wip.patch (obsolete) — Splinter Review
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).
(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.
Attached patch start.patchSplinter Review
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?
Attachment #8769775 - Attachment is obsolete: true
Attachment #8770168 - Flags: review?(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).
Attachment #8770168 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/535a3e0e2903
Baldr: Implement wasm start section; r=luke
Attached patch isexported.patchSplinter Review
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).
Attachment #8770466 - Flags: review?(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).
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.
OK, as you prefer.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8770466 - Flags: review?(luke)
Alon, with this checked in yesterday (should be in today's nightly), you can use the start section in wasm modules.
You need to log in before you can comment on or make changes to this bug.