Baldr: make WebAssembly.Module structured cloneable

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
3 years ago
9 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Depends on 2 bugs, {dev-doc-needed})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected)

Details

(Whiteboard: [leave open])

Attachments

(8 attachments, 11 obsolete attachments)

24.91 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
5.98 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
31.65 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
10.89 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
6.32 KB, patch
terrence
: review+
Details | Diff | Splinter Review
22.40 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
13.52 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
16.45 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
The meaning is equivalent to storing the original bytecode and compiling a new WebAssembly.Module on deserialization.  Additionally, the machine code will be stored (using today's asm.js implicit caching serialization support).  If the buildid/cpuid change between serialization/deserialization, new code will be compiled when deserializing.
Assignee

Comment 1

3 years ago
With the previous de-GC-ification of Module/Metadata, we can trivially remove the 'cx' argument of deserialize() which is good b/c we need to deserialize from a non-JS background thread.
Attachment #8766596 - Flags: review?(bbouvier)
Assignee

Comment 2

3 years ago
Posted patch mv-machine-id (obsolete) — Splinter Review
This patch simply moves MachineId into Module so that it can be serialized/deserialized like everything else, allowing the client to compare and handle failure in its own way (recompiling from source for asm.js, recompiling from the Module's bytecode for wasm).
Attachment #8766650 - Flags: review?(bbouvier)
Assignee

Updated

3 years ago
Depends on: 1284056
Assignee

Updated

3 years ago
Attachment #8766650 - Attachment is obsolete: true
Attachment #8766650 - Flags: review?(bbouvier)
Comment on attachment 8766596 [details] [diff] [review]
rm-deserialize-cx

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

Looks good, thanks!

::: js/src/asmjs/WasmTypes.h
@@ +71,5 @@
>  
>  #define WASM_DECLARE_SERIALIZABLE(Type)                                         \
>      size_t serializedSize() const;                                              \
>      uint8_t* serialize(uint8_t* cursor) const;                                  \
> +    const uint8_t* deserialize(const uint8_t* cursor);    \

style nit: you might want to re-align the trailing \, here and below 2 times.
Attachment #8766596 - Flags: review?(bbouvier) → review+
Assignee

Updated

3 years ago
Whiteboard: [leave open]

Comment 4

3 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c8c1c3ddfb
Baldr: remove unnecessary 'cx' arg from deserialize path (r=bbouvier)
Posted patch WASM + IDB test (obsolete) — Splinter Review
Luke, we want this test to pass, right?
Attachment #8770492 - Flags: review?(luke)
Comment on attachment 8770492 [details] [diff] [review]
WASM + IDB test

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

I'll let Luke do the review, but yes, this looks like the kind of test we want, yes. (sorry for the drive-by review, I was a bit excited and curious)

::: dom/indexedDB/test/test_wasm.html
@@ +48,5 @@
> +    return;
> +  }
> +
> +  var test = tests.shift();
> +  test();

Missing a recursive call to next() here, right?
> Missing a recursive call to next() here, right?

No... each test function should call 'next()'.
Assignee

Comment 9

3 years ago
Comment on attachment 8770492 [details] [diff] [review]
WASM + IDB test

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

Yes, exactly!
Attachment #8770492 - Flags: review?(luke) → review+
Assignee

Comment 10

3 years ago
Trivial patch, provides ideal wasm::Compile signature for next patch.
Attachment #8770823 - Flags: review?(bbouvier)
Assignee

Comment 11

3 years ago
To allow any thread to get a valid Assumptions object, we need the signal setting process-wide.
Attachment #8770824 - Flags: review?(bbouvier)
Assignee

Comment 12

3 years ago
Posted patch pass-build-idSplinter Review
This passes the build-id by value instead of a function-pointer since that will be more convenient for the caller in the next patch (who can simply call mozilla::GetBuildId themselves).
Attachment #8770825 - Flags: review?(bbouvier)
Assignee

Comment 13

3 years ago
Posted patch add-pr-file-desc (obsolete) — Splinter Review
This patch just adds the stubs to PosixNSPR.h so that everything can compile and link.  When shell tests are added that actually use this, these methods would be implemented.  But for now, it allows quickly testing in Gecko.
Attachment #8770827 - Flags: review?(terrence)
Assignee

Comment 14

3 years ago
Attachment #8770827 - Attachment is obsolete: true
Attachment #8770827 - Flags: review?(terrence)
Attachment #8770829 - Flags: review?(terrence)
Assignee

Comment 15

3 years ago
Posted patch wasm-background-serializable (obsolete) — Splinter Review
Here's a first draft of the JSAPIs we talked about.  The build-id (which should come from mozilla::GetBuildId()) must be passed to deserialize (so that it may be compared with what was pulled out of the file).  Beware of bugs; I haven't been able to run the patch (but if everything compiles, what could go wrong?).  (Note the above 4 patches have to be applied first.)
Attachment #8770831 - Flags: feedback?(amarchesini)
Comment on attachment 8770829 [details] [diff] [review]
add-pr-file-desc

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

I think I'd prefer something in mfbt instead of more NSPR usage, as it seems we'll need to implement a non-posix version anyway; see, for example, bug 1237761. Alternatively, it would be nice to rip out PosixNSPR altogether: it's not nearly as necessary anymore now that it's trivial to build NSPR alongside the normal build.

I'd also like a pony while you're at it. If this unblocks WebAssembly then I'm fine taking it.

::: js/src/vm/PosixNSPR.cpp
@@ +372,5 @@
>  
> +int32_t
> +PR_FileDesc2NativeHandle(PRFileDesc* fd)
> +{
> +    MOZ_CRASH("NYI");

If you're not folding in the implementation before landing, please spell these out.
Attachment #8770829 - Flags: review?(terrence) → review+
Assignee

Comment 17

3 years ago
(In reply to Terrence Cole [:terrence] from comment #16)
I'd also love to see all of this in MFBT, but I think that's a bit bigger than me :)  Will replace NYI with real names.
Attachment #8770823 - Flags: review?(bbouvier) → review+
Comment on attachment 8770824 [details] [diff] [review]
process-wide-signals

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

::: js/src/asmjs/WasmSignalHandlers.h
@@ +36,5 @@
>  InterruptRunningJitCode(JSRuntime* rt);
>  
>  namespace wasm {
>  
> +// Ensure the given JSRuntime is setup to use signals. Failure to enable signal

not sure, but shouldn't this be "set up" in two words?

@@ +37,5 @@
>  
>  namespace wasm {
>  
> +// Ensure the given JSRuntime is setup to use signals. Failure to enable signal
> +// handlers indicates some catostrophic failure and creation of the runtime must

nit: catastrophic

@@ +50,5 @@
> +HaveSignalHandlers();
> +
> +// Artificially disable signal handlers, for testing purposes.
> +void
> +DisableSignalHandlersForTesting(bool disable);

It'd be weird to read:

DisableSignalHandlers(false);

cause we're not sure if we're not disabling or disabling; can you rename to ToggleSignalHandlersForTesting, instead? and the JS testing function too?

::: js/src/vm/Runtime.cpp
@@ -273,5 @@
> -SignalBasedTriggersDisabled()
> -{
> -  // Don't bother trying to cache the getenv lookup; this should be called
> -  // infrequently.
> -  return !!getenv("JS_DISABLE_SLOW_SCRIPT_SIGNALS") || !!getenv("JS_NO_SIGNALS");

If you remove support for the former, can you remove support in mach too? http://searchfox.org/mozilla-central/search?q=JS_DISABLE_SLOW_SCRIPT_SIGNALS&path=
Attachment #8770824 - Flags: review?(bbouvier) → review+
Comment on attachment 8770825 [details] [diff] [review]
pass-build-id

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

This code duplication is a bit sad, but well. Please re-enable baseline testing support and r=me.

::: js/src/asmjs/WasmCompile.cpp
@@ -1387,5 @@
>  
> -bool
> -CompileArgs::init(ExclusiveContext* cx)
> -{
> -    alwaysBaseline = cx->options().wasmAlwaysBaseline();

Please show some love to the wasm baseline compiler and don't disable its testing again :)

::: js/src/asmjs/WasmTypes.cpp
@@ +325,2 @@
>  #else
> +# error "unknown architecture"

Can you make a separate #elif defined(JS_CODEGEN_ARM64) with a MOZ_CRASH("NYI") here? (otherwise I think arm64 won't compile)
Attachment #8770825 - Flags: review?(bbouvier) → review+
Assignee

Comment 20

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #18)
> It'd be weird to read:
> 
> DisableSignalHandlers(false);
> 
> cause we're not sure if we're not disabling or disabling; can you rename to
> ToggleSignalHandlersForTesting, instead? and the JS testing function too?

You're right, but the reason is that the reality is actually complex since we're not offering a way to turn *on* signals (if signal handling isn't supported in the first place); we're only offering a way to disable them if they were on in the first place.  So the double-negative is actually saying something different than the positive here ;)  But maybe "disable" is to generic; probably "suppress" is better.

> If you remove support for the former, can you remove support in mach too?
> http://searchfox.org/mozilla-central/
> search?q=JS_DISABLE_SLOW_SCRIPT_SIGNALS&path=

Oops, that was a mistake, I intended to move those checks into ProcessHasSignalHandlers().  Good catch!
Assignee

Comment 21

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #19)
Oh dear; I even *specifically thought* when cutting that code "don't get distracted and forget to paste it back at the callers".  You're right though, the code duplication really is too much.  All that's really needed is an *alternative* path to initialize with a given BuildId (and reuse the GetCPUID()), so I'll keep the init-from-cx path (to avoid all the duplication) and just add one that lets you pass a BuildId.  I'll do that.

Comment 22

3 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7792f6453b84
Baldr: make Module RefCounted (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40efa3603e0
Baldr: change wasm::Compile to take ShareableBytes (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/606e6f6aebe2
Baldr: use process-wide signal-handling-support query (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/09461dda5ac8
Baldr: provide alternative path that provides build-id (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a5424096d5
Add subset of prio.h to PosixNSPR.h so PRFileDesc can be used in SpiderMonkey (r=terrence)
Posted patch IDB + WASM - part 1 (obsolete) — Splinter Review
This patch does these things:

1. if add/put is called with some WASM module, store a MutableFile instead and we return it back to the IDBRequest. No serialization is done yet.

2. When get is called. The WASM MutableFile is returned to the IDBRequest. No deserialization is done yet.
Attachment #8775897 - Flags: review?(jvarga)
Assignee

Comment 25

3 years ago
Posted patch wasm-background-serializable (obsolete) — Splinter Review
(Rebased to mozilla-inbound.)
Attachment #8770831 - Attachment is obsolete: true
Attachment #8770831 - Flags: feedback?(amarchesini)

Updated

3 years ago
Attachment #8775897 - Flags: review?(jvarga) → feedback?(jvarga)
Comment on attachment 8775897 [details] [diff] [review]
IDB + WASM - part 1

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

I applied all the needed patches. I think I now understand how things are done.
baku's patch for WASM in IDB looks good, I didn't review it, I just verified rough design.
Anyway, I see other challenges ... The current serialize/deserialize methods in JS expect
just a file descriptor and they create a memory mapping, so the rest can just operate
on current memory position (just like a contiguous memory buffer). The methods operate on
current thread.
We could somehow expose PRFileDesc in FileHandle API, but we would also need to create I/O
threads for serialization/deserialization. Probably also a manager for threads, etc.
Not sure if JS engine can do it instead. Then we would need to do quota checks on the child side.
All the threading and quota checks are already implemented on the parent side for FileHandle API.
So if there are multiple deserialization requests we can handle them in parallel. I think we can
even do reading of the same file in parallel.
The other option would be to just create buffers by IDB + FileHandle and handle them to JS.
That can be a problem for big WASM modules.
Maybe it would be better to rework WASM serialization/deserialization to take chunks.
I'm not sure if that's a lot of work, but I think not depending on contiguous buffers is always
better.
We found out that chunks would be better for normal IDB operations too. See bug 1274075, comment
58 and 59.
Attachment #8775897 - Flags: feedback?(jvarga) → feedback+
Assignee

Comment 27

3 years ago
The contiguity is pretty useful for another reason (in addition to making it simple to simply splat out a bunch of bytes efficiently): once we finish our internal work to avoid code patching, on cache hit, we won't have to do any blocking file i/o of the main code section after the PR_MemMap.  This should make cache hits really fast even for large cached modules and make wasm more like native dynamic loading.

What thread would serialize()/deserialize() get called on currently?

To handle the quota/growth case (when a cache hit needs to re-compile and rewrite the file), can we instead do the callback into the browser requesting to resize (instead of the current ftruncate()) as we discussed a while ago?  Then the JS engine would only ever be writing into a file whose size was allocated by the parent process.
(In reply to Luke Wagner [:luke] from comment #27)
> What thread would serialize()/deserialize() get called on currently?

The main thread or a DOM worker in content process, no ?
We can also use the stream transport service I think on the child side.
Assignee

Comment 30

3 years ago
(In reply to Jan Varga [:janv] from comment #28)
> (In reply to Luke Wagner [:luke] from comment #27)
> > What thread would serialize()/deserialize() get called on currently?
> 
> The main thread or a DOM worker in content process, no ?

Well I had thought that while we call the IDB function on the main/worker-thread, that would dispatch something to some PBackground/IO thread where the actual WasmBackgroundSerializable::serialize()/DeserializeWasmModuleObject() is called (hence the name :).
(In reply to Luke Wagner [:luke] from comment #27)
> The contiguity is pretty useful for another reason (in addition to making it
> simple to simply splat out a bunch of bytes efficiently): once we finish our
> internal work to avoid code patching, on cache hit, we won't have to do any
> blocking file i/o of the main code section after the PR_MemMap.  This should
> make cache hits really fast even for large cached modules and make wasm more
> like native dynamic loading.
> 
> What thread would serialize()/deserialize() get called on currently?
> 
> To handle the quota/growth case (when a cache hit needs to re-compile and
> rewrite the file), can we instead do the callback into the browser
> requesting to resize (instead of the current ftruncate()) as we discussed a
> while ago?  Then the JS engine would only ever be writing into a file whose
> size was allocated by the parent process.

Ok, I see your point. Now I wonder if we actually want to integrate most of AsmJSCache.cpp into IDB, so the final object on the child side would be a file descriptor. Then we don't need FileHandle API at all.
Assignee

Comment 32

3 years ago
(In reply to Jan Varga [:janv] from comment #31)
> Ok, I see your point. Now I wonder if we actually want to integrate most of
> AsmJSCache.cpp into IDB, so the final object on the child side would be a
> file descriptor. Then we don't need FileHandle API at all.

I'm planning to remove dom/asmjscache within the next year, probably less, so unless there are simple code-reuse/sharing opportunities, I wouldn't waste any time on dom/asmjscache.
Assignee

Comment 33

3 years ago
Posted patch wasm-background-serializable (obsolete) — Splinter Review
Rebased
Attachment #8776602 - Attachment is obsolete: true
Assignee

Comment 34

3 years ago
Posted patch wasm-background-serializable (obsolete) — Splinter Review
Attachment #8791664 - Attachment is obsolete: true
Assignee

Comment 35

3 years ago
Posted patch wasm-background-serializable (obsolete) — Splinter Review
Updated to return "recompiled" outparam instead of re-writing in-place.
Attachment #8791695 - Attachment is obsolete: true
Assignee

Comment 36

3 years ago
Posted patch wasm-background-serializable (obsolete) — Splinter Review
(Clean out some unnecessary #includes.)
Attachment #8792519 - Attachment is obsolete: true

Updated

3 years ago
Depends on: 964561

Updated

3 years ago
Attachment #8770492 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8775897 - Attachment is obsolete: true

Updated

3 years ago
Depends on: 1311466

Updated

3 years ago
No longer depends on: 964561
I created a separate bug 1311466 for IDB related stuff since it's about 10 patches with more to come.
I reused some stuff from IDB patches (code and tests) that were submitted here. Thank for that!
Posted patch wasm-background-serializable (obsolete) — Splinter Review
WasmModule.cpp now needs a "asmjs/WasmCompile.h" include
Attachment #8792521 - Attachment is obsolete: true
Assignee

Comment 39

3 years ago
The preparatory patch hoists the Assumptions out of Metadata and into the Module.  This just makes sense since Assumptions are only needed for (de)serialization and can die with the Module.  Additionally, the assumptions are now checked explicitly for deserializing the module which is what I initially did and then accidentally regressed when refactoring asm.js into wasm.
Attachment #8802834 - Attachment is obsolete: true
Attachment #8803182 - Flags: review?(bbouvier)
Assignee

Comment 40

3 years ago
This preparatory patch splits out bytecode from "compiled code" (machine code and all the stuff that depends on it).  The reason is that IDB will store these in separate files.
Attachment #8803185 - Flags: review?(bbouvier)
Assignee

Comment 41

3 years ago
Posted patch add-jsapiSplinter Review
See comments in jsapi.h.
Attachment #8803192 - Flags: review?(bbouvier)
Comment on attachment 8803182 [details] [diff] [review]
hoist-assumptions

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

Sweet!

::: js/src/asmjs/WasmModule.cpp
@@ +291,5 @@
>  
> +/* static */ bool
> +Module::assumptionsMatch(const Assumptions& assumptions, const uint8_t* cursor, size_t limit)
> +{
> +    return Assumptions::match(assumptions, cursor, limit);

Is this function really useful? I guess it makes sense if we want the Module to be the only thing exposed to the serializing code.

::: js/src/asmjs/WasmTypes.h
@@ +931,5 @@
>  
>      bool operator==(const Assumptions& rhs) const;
>      bool operator!=(const Assumptions& rhs) const { return !(*this == rhs); }
>  
> +    // TODO

TODO //
Attachment #8803182 - Flags: review?(bbouvier) → review+
Comment on attachment 8803185 [details] [diff] [review]
split-module-file

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

Thanks!

::: js/src/asmjs/AsmJS.cpp
@@ +8482,5 @@
> +    cursor = WriteScalar<size_t>(cursor, compiledSize);
> +
> +    uint8_t* bytecodeBegin = cursor;
> +    uint8_t* compiledBegin = bytecodeBegin + bytecodeSize;
> +    cursor += bytecodeSize + compiledSize;

Can you move this line just after the call to module.serialize? I thought it'd make it easier for understanding.

@@ +8518,5 @@
> +
> +    size_t bytecodeSize;
> +    cursor = ReadScalarFallible<size_t>(cursor, &bytecodeSize, &bytesRemain);
> +    if (!cursor)
> +        return false;

Shouldn't this be |return true|?

@@ +8523,5 @@
> +
> +    size_t compiledSize;
> +    cursor = ReadScalarFallible<size_t>(cursor, &compiledSize, &bytesRemain);
> +    if (!cursor)
> +        return false;

ditto

@@ +8527,5 @@
> +        return false;
> +
> +    const uint8_t* bytecodeBegin = cursor;
> +    const uint8_t* compiledBegin = bytecodeBegin + bytecodeSize;
> +    cursor += bytecodeSize + compiledSize;

Can this be moved just after Module::deserialize?

@@ +8554,5 @@
>      cursor = moduleChars.deserialize(cursor);
>      if (!moduleChars.match(parser))
>          return true;
>  
> +    MOZ_RELEASE_ASSERT(cursor == entry.memory + entry.serializedSize);

If this is just a corrupted cache, we could keep the previous behavior and return true here too, couldn't we?

::: js/src/asmjs/WasmModule.h
@@ +236,5 @@
>  
> +    void serializedSize(size_t* bytecodeSize, size_t* compiledSize) const;
> +    void serialize(uint8_t* bytecodeBegin, size_t bytecodeSize,
> +                   uint8_t* compiledBegin, size_t compiledSize) const;
> +    static bool compiledAssumptionsMatch(const Assumptions& current, const uint8_t* compiledBegin,

(if you keep this function) The name is a bit bizarre, but other combinations don't sound any better (assumptionsMatchCompiled?).
Attachment #8803185 - Flags: review?(bbouvier) → review+
Comment on attachment 8803192 [details] [diff] [review]
add-jsapi

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

Great, looking forward to having this in browser!

What about adding a shell builtin function to test all of this?

::: js/src/asmjs/WasmModule.cpp
@@ +412,5 @@
> +    PRFileMap* map = PR_CreateFileMap(file, info.size, PR_PROT_READONLY);
> +    if (!map)
> +        return nullptr;
> +
> +    uint8_t* memory = (uint8_t*)PR_MemMap(map, 0, info.size);

nit: memory can be nullptr (according to https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/PR_MemMap ), can you add an `if` here?

@@ +416,5 @@
> +    uint8_t* memory = (uint8_t*)PR_MemMap(map, 0, info.size);
> +
> +    // PRFileMap objects do not need to be kept alive after the memory has been
> +    // mapped: the mapping keeps the underlying file alive.
> +    PR_CloseFileMap(map);

nit: can you check the return value of this call?

@@ +425,5 @@
> +bool
> +wasm::CompiledModuleAssumptionsMatch(PRFileDesc* compiled, JS::BuildIdCharVector&& buildId)
> +{
> +    PRFileInfo info;
> +    if (PR_GetOpenFileInfo(compiled, &info) != PR_SUCCESS)

Just making sure here: the name of this function suggests that the file has already been opened. Is that right? Otherwise, we should probably close the file somewhere...

@@ +444,5 @@
> +    PRFileInfo bytecodeInfo;
> +    if (PR_GetOpenFileInfo(bytecodeFile, &bytecodeInfo) != PR_SUCCESS)
> +        return nullptr;
> +
> +    UniqueMapping bytecodeMapping = MapFile(bytecodeFile, bytecodeInfo);

Looks like MapFile could also call PR_GetOpenFileInfo and set PRFileInfo as an outparam.

@@ +457,5 @@
> +        UniqueMapping compiledMapping = MapFile(compiledFile, compiledInfo);
> +        if (!compiledMapping)
> +            return nullptr;
> +
> +        return  Module::deserialize(bytecodeMapping.get(), bytecodeInfo.size,

nit: two spaces between `return` and `Module`

@@ +472,5 @@
> +
> +    MOZ_RELEASE_ASSERT(bytecodeEnd == bytecodeMapping.get() + bytecodeInfo.size);
> +
> +    // TODO: capture these from the scripted caller of the JS method call that
> +    // triggered the deserialization.

The ScriptedCaller could be passed as an rvalue parameter to this function (and JSAPI callers), and JSAPI users may call DescribeScriptedCaller before this?

@@ +483,5 @@
> +
> +    UniqueChars error;
> +    SharedModule module = Compile(*bytecode, Move(args), &error);
> +    if (!module) {
> +        MOZ_ASSERT(!error, "only possible error is out-of-memory");

or the passed bytecode file wasn't valid bytecode, or a former version of the binary format, right?

::: js/src/jsapi.h
@@ +5978,5 @@
> + * - Serialization starts when WebAssembly.Module is passed to the
> + * structured-clone algorithm. JS::GetWasmModule is called on the JSRuntime
> + * thread that initiated the structured clone to get the JS::WasmModule.
> + * This interface is then taken to a background thread where serializedSize()
> + * and serialize() are called to write the object to a file. A module is written

nit: a file => several files. A bit redundant with the next sentence, then?

@@ +5987,5 @@
> + *
> + * - Deserialization starts when the structured clone algorithm encounters a
> + * serialized WebAssembly.Module. On a background thread, the compiled-code file
> + * is opened and CompiledWasmModuleAssumptionsMatch is called to see if it is
> + * still valid (as described above). DeserializeWasmModule then called to

nit: DeserializeWasmModule *is* then called

@@ +5989,5 @@
> + * serialized WebAssembly.Module. On a background thread, the compiled-code file
> + * is opened and CompiledWasmModuleAssumptionsMatch is called to see if it is
> + * still valid (as described above). DeserializeWasmModule then called to
> + * construct a JS::WasmModule (also on the background thread), passing the
> + * bytecode file and, if valid, the compiled-code file. The JS::WasmObject is

maybe precise: "opened" bytecode / compiled-code files?

@@ +6016,5 @@
> +extern JS_PUBLIC_API(bool)
> +CompiledWasmModuleAssumptionsMatch(PRFileDesc* compiled, BuildIdCharVector&& buildId);
> +
> +extern JS_PUBLIC_API(RefPtr<WasmModule>)
> +DeserializeWasmModule(PRFileDesc* bytecode, PRFileDesc* maybeCompiled, BuildIdCharVector&& buildId);

Maybe worth expliciting that returning nullptr indicates either an OOM or invalid files?
Attachment #8803192 - Flags: review?(bbouvier) → review+
Assignee

Comment 45

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #42)
> Is this function really useful? I guess it makes sense if we want the Module
> to be the only thing exposed to the serializing code.

Right, the idea is to keep this as a detail of how modules are (de)serialized and so that it's easy to see the ordering constraint locally (between serialize, deserialize and check).
Assignee

Comment 46

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #43)
> > +    MOZ_RELEASE_ASSERT(cursor == entry.memory + entry.serializedSize);
> 
> If this is just a corrupted cache, we could keep the previous behavior and
> return true here too, couldn't we?

I noticed there are some other MOZ_RELEASE_ASSERT()s around, and we're really not trying to fail gracefully here (there are any number of ways we can crash for a corrupt file), so I was thinking it was more consistent to MOZ_RELEASE_ASSERT() across the board.

> > +    static bool compiledAssumptionsMatch(const Assumptions& current, const uint8_t* compiledBegin,
> 
> (if you keep this function) The name is a bit bizarre, but other
> combinations don't sound any better (assumptionsMatchCompiled?).

Yeah, I suppose so, I'll go back to just assumptionsMatch.
Assignee

Comment 47

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #44)
> > +    uint8_t* memory = (uint8_t*)PR_MemMap(map, 0, info.size);
> 
> nit: memory can be nullptr (according to
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/
> PR_MemMap ), can you add an `if` here?

This is a bit subtle, so I'll add a comment: but we want to unconditionally close the PRFileMap.

> > +    PR_CloseFileMap(map);
> 
> nit: can you check the return value of this call?

This is basically a free() call and, like other VirtualFree()/munmap()/etc, I'm not sure there's any value in checking.  Also, it returns a PRStatus which makes it more verbose to check.

> > +    if (PR_GetOpenFileInfo(compiled, &info) != PR_SUCCESS)
> 
> Just making sure here: the name of this function suggests that the file has
> already been opened. Is that right? Otherwise, we should probably close the
> file somewhere...

Right, the caller has opened the file before passing into the JSAPI and will take close it after.

> > +    SharedModule module = Compile(*bytecode, Move(args), &error);
> > +    if (!module) {
> > +        MOZ_ASSERT(!error, "only possible error is out-of-memory");
> 
> or the passed bytecode file wasn't valid bytecode, or a former version of
> the binary format, right?

Heh, I was *about* to say: that can only happen if we break backwards compatibility, which we should never do, but I forgot about the intermediate transition period between 0xd and 0x1.  So I'll remove the assert for now.

> > + * construct a JS::WasmModule (also on the background thread), passing the
> > + * bytecode file and, if valid, the compiled-code file. The JS::WasmObject is
> 
> maybe precise: "opened" bytecode / compiled-code files?

Technically, since PRFileDesc is an abstraction of a file descriptor, PRFileDesc necessarily represents an open file, but I can change from saying "file" to "file descriptor".

> > +extern JS_PUBLIC_API(RefPtr<WasmModule>)
> > +DeserializeWasmModule(PRFileDesc* bytecode, PRFileDesc* maybeCompiled, BuildIdCharVector&& buildId);
> 
> Maybe worth expliciting that returning nullptr indicates either an OOM or
> invalid files?

As mentioned above, while it is technically possible to have this fail for non-OOM reasons, that shouldn't be observable in production so I think there's effectively only one reason to fail.
Assignee

Comment 48

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #44)
> What about adding a shell builtin function to test all of this?

Yeah, I was planning to do that, but it's not totally trivial since the JS APIs we want to test take a file descriptor.  But I'll get to that soon.

Comment 49

3 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0f339a85d6
Baldr: move assumptions from Metadata to Module (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc32d7a9d319
Baldr: split bytecode from rest of compiled code (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f325d73d76d1
Baldr: add JS API to support structured clone of wasm objects (r=bbouvier)

Comment 50

3 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8613eb3daab2
Baldr: remove accidental MOZ_ALWAYS_TRUE (r=me)
Assignee

Updated

3 years ago
Depends on: 1312808
Assignee

Updated

3 years ago
Depends on: 1312817
Assignee

Updated

3 years ago
Depends on: 1312819
Assignee

Updated

3 years ago
Depends on: 1313176

Updated

3 years ago
Depends on: 1313185
Depends on: 1318039
Assignee

Updated

3 years ago
Depends on: 1318252
Assignee

Updated

3 years ago
Depends on: 1319531
Assignee

Updated

3 years ago
Depends on: 1326280
Depends on: 1330661
Assignee

Updated

2 years ago
Depends on: 1341111
Assignee

Updated

2 years ago
Depends on: 1341114
Assignee

Updated

2 years ago
Depends on: 1341271

Updated

2 years ago
Depends on: 1353543
Assignee

Updated

9 months ago
No longer depends on: 1330661
This might have implications on WA docs, e.g. mechanisms to store modules, etc.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.