Closed Bug 1487475 Opened Last year Closed Last year

stop unconditionally holding bytecode in wasm::Module

Categories

(Core :: Javascript: WebAssembly, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: luke, Assigned: luke)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

With bug 1469395, we no longer need to keep the bytecode alive in the Module for serialization.  For bug 1487113, ideally we'd not be saving bytecode in the alt-data since the HTTP cache will *already* be saving it as the original cached response.

IIUC, there are three remaining uses of bytecode:
 1. Module::instantiate() reads the bytecode to do {data,elem} segment init
 2. when the names section is present, they are pulled from the bytecode
 3. view-source

To address 1: after bug 1486549 lands, we can simply copy over *all* data/elem segments (not just the passive ones) when compiling the Module.  Only these segments would be serialized, not the bytecode.

To address 2: the name section can just be copied, when present.

To address 3 when a Module is deserialized from alt-data, I think we can just rely on the devtools to re-fetch the .wasm via the url (we can assert filenameIsURL for serialized modules).

When there is not a url (!filenameIsURL) and the other conditions are met (names section present || devtools open), we can continue to save the bytecode, but probably stashed moved to DebugState.
Just code motion and renaming in preparation for the next patch.
Assignee: nobody → luke
Attachment #9008930 - Flags: review?(ydelendik)
This patch changes the implementation of all custom sections, including the name section, to be stored directly, without reference to the bytecode (allowing removal in the next patch).  Names are still stored as offsets, but now it's into the payload of the name custom section.

One fundamental change is that now wasm::DebugState is only created when debugEnabled==true, instead of always.  This allows wasm::DebugState to unconditionally store a RefPtr<Module> which simplifies various things.  However it means that now we have to test instance->debugEnabled() *before* calling instance->debug() which is the source of all the vm/Debugger.cpp changes.

One thing that accidentally crept into the patch was changing the SharedModule typedef from RefPtr<Module> to RefPtr<const Module> which is why you have all the widespread const-ification changes.  (I should've, but forgot, to split this into a separate patch.)
Attachment #9008943 - Flags: review?(ydelendik)
Attached patch no-save-bytecodeSplinter Review
With that change, it's pretty easy to turn Module::bytecode_ into Module::debugBytecode_ which is null when !debugEnabled_.
Attachment #9008950 - Flags: review?(ydelendik)
Comment on attachment 9008930 [details] [diff] [review]
move-and-rename-code

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

Looks good.

::: js/src/wasm/WasmTypes.cpp
@@ -915,5 @@
>  
>  UniqueTlsData
>  wasm::CreateTlsData(uint32_t globalDataLength)
>  {
> -    MOZ_ASSERT(globalDataLength % gc::SystemPageSize() == 0);

In the WasmGenerator::finishMetadata globalDataLength alignment was removed. Assuming "as required by the allocator functions" requirement is not applicable anymore.
Attachment #9008930 - Flags: review?(ydelendik) → review+
(In reply to Yury Delendik (:yury) from comment #4)
Good spot, yes, I forgot to mention this; that requirement is from back when global data was allocated after the end of executable code (so that rip-relative addressing could be used to load it; such speed!), which was allocated from a page-allocator, but this scheme went away when modules were shared by instances/threads since there were to be N global data segments for one code segment.
Comment on attachment 9008943 [details] [diff] [review]
save-custom-sections-directly

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

Really good. I just have couple of questions about behavior changes: about realm->isDebuggee() check and script text/message when debugger is not in debug mode.

::: js/src/jit-test/tests/debug/wasm-sourceMappingURL.js
@@ +70,5 @@
>  // binary source is disabled.
>  dbg.allowWasmBinarySource = false;
>  g.eval(`o = new WebAssembly.Instance(new WebAssembly.Module(toWasm('(module (func) (export "a" 0))', 'http://example.org/test2')));`);
>  assertEq(gotScript.format, "wasm");
> +assertEq(gotScript.source.sourceMapURL, 'http://example.org/test2');

Note: it was a protection when debuggers using source maps with generated by JS engine wast. At this point it does not matter, so we can always have it available.

::: js/src/vm/Debugger.cpp
@@ +7642,5 @@
>          return ss->substring(cx_, 0, ss->length());
>      }
>  
> +    ReturnType match(Handle<WasmInstanceObject*> instanceObj) {
> +        return NewStringCopyZ<CanGC>(cx_, "[debugger missing wasm binary-to-text conversion]");

There can be a state when debug mode is not enabled, but source text is accessed, e.g. navigating from the profiler or stack trace. This is a reason why we showing "Restart the page"-warning. After this patch we need to ensure that the frontend (debugger.html) can detect this state and display the appropriate message.

::: js/src/wasm/WasmModule.cpp
@@ -1039,5 @@
>      // for non-developer builds).
>  
> -    const ShareableBytes* maybeBytecode = nullptr;
> -    if (cx->realm()->isDebuggee() || metadata().debugEnabled ||
> -        !metadata().funcNames.empty() || !!metadata().moduleName)

The comment above needs to be adjusted. I'm also not sure what is the role of `cx->realm()->isDebuggee()` -- does it just check the case when debugger tab is closed?
Attachment #9008943 - Flags: review?(ydelendik) → review+
Blocks: 1492301
(In reply to Yury Delendik (:yury) from comment #6)
> I'm also not sure what is the role
> of `cx->realm()->isDebuggee()` -- does it just check the case when debugger
> tab is closed?

I'm not super-clear either: I think it represented when the developer tools were open at all or something.
Comment on attachment 9008950 [details] [diff] [review]
no-save-bytecode

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

lgtm
Attachment #9008950 - Flags: review?(ydelendik) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6493ea25b80c
Baldr: rename and split the ModuleGenerator finish functions (r=yury)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1185e88c0067
Baldr: save custom sections separately of bytecode (r=yury)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03b6055ab32
Baldr: stop saving bytecode in wasm::Module (r=yury)
You need to log in before you can comment on or make changes to this bug.