1326280 - Baldr: just use .wasm as bytecode file format

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: tweak-bytecode-format

Since the bytecode-file of a serialized wasm module must always stay valid (so that one can always recompile), and since .wasm bytecode files obviously have the same requirement, it makes sense to have the bytecode file simply *be* a wasm bytecode file.  The only practical difference from what we have now is that, by using SerializePodVector(bytecode), we prepend the vector length as a uint32_t (which in theory limits us from ever supporting >4gb modules...).

This change shouldn't (badly) break anything: if there were any cached Modules (only in Nightly/DevEd), they will fail to validate the bytecode (in wasm::Compile) which will result in an IDB exception.  The same thing will happen when we change from 0xD->0x1 and, in general, correct usage of IDB should be to delete a cached Module if it throws on get() (this is necessary to handle certain quota corner cases).  bug 1312808 actually adds a recompilation test (that uses cached bytecode via a snapshotted profile directory) that (correctly) throws an exception (now that the bytecode fails to validate) and so that's the only change to wasm_recompile_profile.zip is to strip the first 4 bytes of the bytecode file.

This patch (viz. the !!bytecodeSize == !!bytecodeBegin assert) also caught a sillines in asm.js caching: bytecodeSize is always zero because asm.js Modules have no bytecode.  Removing the bytecodeSize shouldn't break anything because the now-super-robust Assumptions check will fail.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8822504 [details] [diff] [review]
tweak-bytecode-format

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

Good catch and nice simplifications around the AsmJS.cpp code. I like the transfer of responsibilities for backward compatibility here. Thanks for the patch.

Just making sure I understand: bytecodeSize was 1 before for containing the length of the bytecode vector (0), hence the assert not triggering before?

::: js/src/wasm/AsmJS.cpp
@@ +8443,1 @@
>                              moduleChars.serializedSize();

pre-existing: should we make sure this doesn't overflow? (I think in the worst case this will cache an invalid entry that will be invalidated when re-reading, but we might as well just do something here)

Comment 2

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Just making sure I understand: bytecodeSize was 1 before for containing the
> length of the bytecode vector (0), hence the assert not triggering before?

Yes, exactly (except bytecodeSize was 4, because uint32_t not varu32 :).

> ::: js/src/wasm/AsmJS.cpp
> @@ +8443,1 @@
> >                              moduleChars.serializedSize();
> 
> pre-existing: should we make sure this doesn't overflow?

I think it can't because for the size_t to overflow, we'd have to be actually using more memory than we have address space :)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43dac5318d4
Baldr: use .wasm as bytecode file format (r=bbouvier)

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8822504 [details] [diff] [review]
tweak-bytecode-format

Approval Request Comment
[Feature/Bug causing the regression]: enhancement, not a regression fix
[User impact if declined]: would *become* a regression when 53 ships
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: affects still-default-off wasm feature

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[Julien Cristau [:jcristau]]

Comment on attachment 8822504 [details] [diff] [review]
tweak-bytecode-format

wasm cache format tweak for aurora52

Comment 7

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 8

[Luke Wagner [:luke]]

Oops, sorry, I should have noticed and mentioned that the patch in this bug needs to be applied on top of the patch in bug 1312808.  (Just confirmed locally.)

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4cf9ed24f6b

Comment 10

[Luke Wagner [:luke]]

Thanks!