Closed Bug 1326280 Opened 4 years ago Closed 4 years ago
Baldr: just use .wasm as bytecode file 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.
Attachment #8822504 - Flags: review?(bbouvier)
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)
Attachment #8822504 - Flags: review?(bbouvier) → review+
(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 :)
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f43dac5318d4 Baldr: use .wasm as bytecode file format (r=bbouvier)
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
Attachment #8822504 - Flags: approval-mozilla-aurora?
Comment on attachment 8822504 [details] [diff] [review] tweak-bytecode-format wasm cache format tweak for aurora52
Attachment #8822504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora
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.)
Flags: needinfo?(luke) → needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.