1276029 - Baldr: make WebAssembly.Module structured cloneable

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P5)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[Luke Wagner [:luke]]

Attachment: rm-deserialize-cx

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.

Comment 2

[Luke Wagner [:luke]]

Attachment: mv-machine-id

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).

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c8c1c3ddfb
Baldr: remove unnecessary 'cx' arg from deserialize path (r=bbouvier)

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a4c8c1c3ddfb

Comment 6

[Andrea Marchesini [:baku]]

Attachment: WASM + IDB test

Luke, we want this test to pass, right?

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 8

[Andrea Marchesini [:baku]]

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

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

Comment 9

[Luke Wagner [:luke]]

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

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

Yes, exactly!

Comment 10

[Luke Wagner [:luke]]

Attachment: pass-shared-bytes

Trivial patch, provides ideal wasm::Compile signature for next patch.

Comment 11

[Luke Wagner [:luke]]

Attachment: process-wide-signals

To allow any thread to get a valid Assumptions object, we need the signal setting process-wide.

Comment 12

[Luke Wagner [:luke]]

Attachment: pass-build-id

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).

Comment 13

[Luke Wagner [:luke]]

Attachment: add-pr-file-desc

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.

Comment 14

[Luke Wagner [:luke]]

Attachment: add-pr-file-desc

Comment 15

[Luke Wagner [:luke]]

Attachment: wasm-background-serializable

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.)

Comment 16

[Terrence Cole [:terrence]]

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.

Comment 17

[Luke Wagner [:luke]]

(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.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

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=

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

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)

Comment 20

[Luke Wagner [:luke]]

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

Comment 21

[Luke Wagner [:luke]]

(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

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7792f6453b84
https://hg.mozilla.org/mozilla-central/rev/c40efa3603e0
https://hg.mozilla.org/mozilla-central/rev/606e6f6aebe2
https://hg.mozilla.org/mozilla-central/rev/09461dda5ac8
https://hg.mozilla.org/mozilla-central/rev/86a5424096d5

Comment 24

[Andrea Marchesini [:baku]]

Attachment: IDB + WASM - part 1

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.

Comment 25

[Luke Wagner [:luke]]

Attachment: wasm-background-serializable

(Rebased to mozilla-inbound.)

Comment 26

[Jan Varga [:janv]]

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.

Comment 27

[Luke Wagner [:luke]]

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.

Comment 28

[Jan Varga [:janv]]

(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 ?

Comment 29

[Jan Varga [:janv]]

We can also use the stream transport service I think on the child side.

Comment 30

[Luke Wagner [:luke]]

(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 :).

Comment 31

[Jan Varga [:janv]]

(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.

Comment 32

[Luke Wagner [:luke]]

(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.

Comment 33

[Luke Wagner [:luke]]

Attachment: wasm-background-serializable

Rebased

Comment 34

[Luke Wagner [:luke]]

Attachment: wasm-background-serializable

Comment 35

[Luke Wagner [:luke]]

Attachment: wasm-background-serializable

Updated to return "recompiled" outparam instead of re-writing in-place.

Comment 36

[Luke Wagner [:luke]]

Attachment: wasm-background-serializable

(Clean out some unnecessary #includes.)

Comment 37

[Jan Varga [:janv]]

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!

Comment 38

[Jan Varga [:janv]]

Attachment: wasm-background-serializable

WasmModule.cpp now needs a "asmjs/WasmCompile.h" include

Comment 39

[Luke Wagner [:luke]]

Attachment: hoist-assumptions

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.

Comment 40

[Luke Wagner [:luke]]

Attachment: split-module-file

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.

Comment 41

[Luke Wagner [:luke]]

Attachment: add-jsapi

See comments in jsapi.h.

Comment 42

[Benjamin Bouvier [:bbouvier] (inactive)]

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 //

Comment 43

[Benjamin Bouvier [:bbouvier] (inactive)]

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?).

Comment 44

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 45

[Luke Wagner [:luke]]

(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).

Comment 46

[Luke Wagner [:luke]]

(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.

Comment 47

[Luke Wagner [:luke]]

(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.

Comment 48

[Luke Wagner [:luke]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8613eb3daab2
Baldr: remove accidental MOZ_ALWAYS_TRUE (r=me)

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b0f339a85d6
https://hg.mozilla.org/mozilla-central/rev/cc32d7a9d319
https://hg.mozilla.org/mozilla-central/rev/f325d73d76d1
https://hg.mozilla.org/mozilla-central/rev/8613eb3daab2

Comment 52

[Chris Mills [:cmills]]

This might have implications on WA docs, e.g. mechanisms to store modules, etc.

Comment 53

[Benjamin Bouvier [:bbouvier] (inactive)]

Luke, is this still of interest?

Comment 54

[Luke Wagner [:luke]]

I think this landed but I forgot to remove the [leave-open].