Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2405153003:
[wasm] support for recompilation if deserialization fails (Closed)
|
Created:
4 years, 2 months ago by Mircea Trofin Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, jbroman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wasm] support for recompilation if deserialization fails
When serializing wasm, append the uncompiled bytes
explicitly. Use them when deserializing to ensure
structured cloning succeeds even after chrome updates.
Currently, the canonical uncompiled bytes representation
is bytes - stored as a String object on the v8 side.
Eventually, we want to move to a less memory-intensive
option, such as Blobs. There are challenges with adopting
a different representation, one being that Blobs aren't a
V8 concept. In addition, we may want to explore that
option in conjunction with adding support for streamed
compilation, since the choice of APIs there may impact
choices we make for internal representation of uncompiled
bytes, and it'd be more maintainable to have one common
such representation, if possible.
Meanwhile, we store the payload as raw bytes, and tag that
payload to make sure we can later introduce a different
representation while still supporting the current one.
BUG=
Committed: https://crrev.com/15aaa54e5fe36c2d2bf1a53e1410be5ea274fec6
Cr-Commit-Position: refs/heads/master@{#426268}
Patch Set 1 #Patch Set 2 : behavior when breaking changes #
Total comments: 13
Patch Set 3 : post-feedback #Patch Set 4 : post-feedback #Patch Set 5 : IndexedDB tests #Patch Set 6 : cleaner invalidation #
Total comments: 1
Patch Set 7 : uncompiled->wire #Messages
Total messages: 57 (37 generated)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In the meantime, we store the payload as raw bytes, and tag it to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to validate the data flow is correct. BUG= ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org, jochen@chromium.org, titzer@chromium.org
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In the meantime, we store the payload as raw bytes, and tag it to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to validate the data flow is correct. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In the meantime, we store the payload as raw bytes, and tag it to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to validate the data flow is correct. BUG= ==========
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In the meantime, we store the payload as raw bytes, and tag it to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to validate the data flow is correct. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to be concerned with just data flow. Hence, no additional tests. BUG= ==========
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to be concerned with just data flow. Hence, no additional tests. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to be concerned with just data flow. Hence, no additional tests here. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2033: CHECK(!retval.IsEmpty()); I don't think we should crash here. Downgrading is very common, e.g. ppl might run stable and beta in parallel. If we just can't compile the blob, we should throw an exception. could you embed a version number in the serialized blob, so we don't even attempt to deserialize old, incompatible blobs?
jbroman@chromium.org changed reviewers: + jbroman@chromium.org
I hope you don't mind if I drop in here. https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1283: m_writer.append(StringTag); At the moment, StringTag generally means a UTF-8 string. It might be clearer to have a separate enum for WasmModuleStorageTag or something like that, to avoid possible confusion. https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2007: std::unique_ptr<const uint8_t[]>(uncompiledBytesStart), Not new to this CL, but please don't put unowned data in an std::unique_ptr like this. If it doesn't take ownership of the data, it seems to me that this is an indication that v8::WasmCompiledModule::UncompiledBytes should take a const uint8_t* rather than a std::unique_ptr<const uint8_t[]>.
https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); UncompiledBytes isn't a very good name. These are the original (encoded) bytes of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" is a better name?
https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); On 2016/10/12 15:09:53, titzer wrote: > UncompiledBytes isn't a very good name. These are the original (encoded) bytes > of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" is a > better name? I wanted to draw the compiled vs uncompiled distinction. "Original" may be confusing because it may also mean "the object we serialized", for example. If the application does some form of local patching, "Original" may mean "what was downloaded" (vs what ended up being patched". I'd prefer keeping "UncompiledBytes", I think it clarifies what his is, especially in the context of "DeserializeOrCompile". https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1283: m_writer.append(StringTag); On 2016/10/12 15:05:38, jbroman wrote: > At the moment, StringTag generally means a UTF-8 string. It might be clearer to > have a separate enum for WasmModuleStorageTag or something like that, to avoid > possible confusion. OK, and we're OK with later using the blob tag if that ends up being exactly how we store, correct? On the new tag name: the data there is raw bytes, it so happens we pull it from a String object at serialization. We do that t avoid copying it on the v8's Serialize API side. Then, at deserialization, we end up treating it like a byte* anyway. How about "RawBytesTag"? https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2007: std::unique_ptr<const uint8_t[]>(uncompiledBytesStart), On 2016/10/12 15:05:38, jbroman wrote: > Not new to this CL, but please don't put unowned data in an std::unique_ptr like > this. > > If it doesn't take ownership of the data, it seems to me that this is an > indication that v8::WasmCompiledModule::UncompiledBytes should take a const > uint8_t* rather than a std::unique_ptr<const uint8_t[]>. I saw the todo here, I'll update on the v8 side and re-update this CL. Thanks for reminding me! https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2033: CHECK(!retval.IsEmpty()); On 2016/10/12 09:34:59, jochen wrote: > I don't think we should crash here. Downgrading is very common, e.g. ppl might > run stable and beta in parallel. If we just can't compile the blob, we should > throw an exception. Is it an exception, or is it just returning "false"? (I'm assuming the latter) > > could you embed a version number in the serialized blob, so we don't even > attempt to deserialize old, incompatible blobs? The serialized blob has a version number in the header. The deserialization logic checks that first and does not go any further if the version is not a match for the current version. This all happens on the v8 side.
https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); On 2016/10/12 16:01:07, Mircea Trofin wrote: > On 2016/10/12 15:09:53, titzer wrote: > > UncompiledBytes isn't a very good name. These are the original (encoded) bytes > > of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" is > a > > better name? > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > confusing because it may also mean "the object we serialized", for example. If > the application does some form of local patching, "Original" may mean "what was > downloaded" (vs what ended up being patched". I'd prefer keeping > "UncompiledBytes", I think it clarifies what his is, especially in the context > of "DeserializeOrCompile". "Uncompiled" doesn't suggest to me that this is encoded module data that came over the wire.
On 2016/10/12 16:15:31, titzer wrote: > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp > (right): > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: > v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); > On 2016/10/12 16:01:07, Mircea Trofin wrote: > > On 2016/10/12 15:09:53, titzer wrote: > > > UncompiledBytes isn't a very good name. These are the original (encoded) > bytes > > > of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" > is > > a > > > better name? > > > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > > confusing because it may also mean "the object we serialized", for example. If > > the application does some form of local patching, "Original" may mean "what > was > > downloaded" (vs what ended up being patched". I'd prefer keeping > > "UncompiledBytes", I think it clarifies what his is, especially in the context > > of "DeserializeOrCompile". > > "Uncompiled" doesn't suggest to me that this is encoded module data that came > over the wire. How about "WasmEncodedBytes" then?
On 2016/10/12 16:15:31, titzer wrote: > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp > (right): > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: > v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); > On 2016/10/12 16:01:07, Mircea Trofin wrote: > > On 2016/10/12 15:09:53, titzer wrote: > > > UncompiledBytes isn't a very good name. These are the original (encoded) > bytes > > > of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" > is > > a > > > better name? > > > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > > confusing because it may also mean "the object we serialized", for example. If > > the application does some form of local patching, "Original" may mean "what > was > > downloaded" (vs what ended up being patched". I'd prefer keeping > > "UncompiledBytes", I think it clarifies what his is, especially in the context > > of "DeserializeOrCompile". > > "Uncompiled" doesn't suggest to me that this is encoded module data that came > over the wire. Sorry to be a stickler about this :/ Uncompiled just suggests what hasn't happened to the module yet, whereas I think we should have a name that more directly describes what it actually is.
On 2016/10/12 16:16:44, Mircea Trofin wrote: > On 2016/10/12 16:15:31, titzer wrote: > > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp > > (right): > > > > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: > > v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); > > On 2016/10/12 16:01:07, Mircea Trofin wrote: > > > On 2016/10/12 15:09:53, titzer wrote: > > > > UncompiledBytes isn't a very good name. These are the original (encoded) > > bytes > > > > of the module, correct? Then maybe "OriginalBytes" or > "OriginalModuleBytes" > > is > > > a > > > > better name? > > > > > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > > > confusing because it may also mean "the object we serialized", for example. > If > > > the application does some form of local patching, "Original" may mean "what > > was > > > downloaded" (vs what ended up being patched". I'd prefer keeping > > > "UncompiledBytes", I think it clarifies what his is, especially in the > context > > > of "DeserializeOrCompile". > > > > "Uncompiled" doesn't suggest to me that this is encoded module data that came > > over the wire. > > How about "WasmEncodedBytes" then? WasmWireBytes? WasmModuleBytes?
On 2016/10/12 17:09:16, titzer wrote: > On 2016/10/12 16:16:44, Mircea Trofin wrote: > > On 2016/10/12 16:15:31, titzer wrote: > > > > > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: > > > v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); > > > On 2016/10/12 16:01:07, Mircea Trofin wrote: > > > > On 2016/10/12 15:09:53, titzer wrote: > > > > > UncompiledBytes isn't a very good name. These are the original (encoded) > > > bytes > > > > > of the module, correct? Then maybe "OriginalBytes" or > > "OriginalModuleBytes" > > > is > > > > a > > > > > better name? > > > > > > > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > > > > confusing because it may also mean "the object we serialized", for > example. > > If > > > > the application does some form of local patching, "Original" may mean > "what > > > was > > > > downloaded" (vs what ended up being patched". I'd prefer keeping > > > > "UncompiledBytes", I think it clarifies what his is, especially in the > > context > > > > of "DeserializeOrCompile". > > > > > > "Uncompiled" doesn't suggest to me that this is encoded module data that > came > > > over the wire. > > > > How about "WasmEncodedBytes" then? > > WasmWireBytes? > WasmModuleBytes? I like WasmWireBytes!
On 2016/10/12 17:05:14, titzer wrote: > On 2016/10/12 16:15:31, titzer wrote: > > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp > > (right): > > > > > https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: > > v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); > > On 2016/10/12 16:01:07, Mircea Trofin wrote: > > > On 2016/10/12 15:09:53, titzer wrote: > > > > UncompiledBytes isn't a very good name. These are the original (encoded) > > bytes > > > > of the module, correct? Then maybe "OriginalBytes" or > "OriginalModuleBytes" > > is > > > a > > > > better name? > > > > > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > > > confusing because it may also mean "the object we serialized", for example. > If > > > the application does some form of local patching, "Original" may mean "what > > was > > > downloaded" (vs what ended up being patched". I'd prefer keeping > > > "UncompiledBytes", I think it clarifies what his is, especially in the > context > > > of "DeserializeOrCompile". > > > > "Uncompiled" doesn't suggest to me that this is encoded module data that came > > over the wire. > > Sorry to be a stickler about this :/ > Uncompiled just suggests what hasn't happened to the module yet, whereas I think > we should have a name that more directly describes what it actually is. I also think API names are important, I value this discussion :)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1272: v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes(); On 2016/10/12 16:15:30, titzer wrote: > On 2016/10/12 16:01:07, Mircea Trofin wrote: > > On 2016/10/12 15:09:53, titzer wrote: > > > UncompiledBytes isn't a very good name. These are the original (encoded) > bytes > > > of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" > is > > a > > > better name? > > > > I wanted to draw the compiled vs uncompiled distinction. "Original" may be > > confusing because it may also mean "the object we serialized", for example. If > > the application does some form of local patching, "Original" may mean "what > was > > downloaded" (vs what ended up being patched". I'd prefer keeping > > "UncompiledBytes", I think it clarifies what his is, especially in the context > > of "DeserializeOrCompile". > > "Uncompiled" doesn't suggest to me that this is encoded module data that came > over the wire. Done. https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1283: m_writer.append(StringTag); On 2016/10/12 16:01:07, Mircea Trofin wrote: > On 2016/10/12 15:05:38, jbroman wrote: > > At the moment, StringTag generally means a UTF-8 string. It might be clearer > to > > have a separate enum for WasmModuleStorageTag or something like that, to avoid > > possible confusion. > > OK, and we're OK with later using the blob tag if that ends up being exactly how > we store, correct? > > On the new tag name: the data there is raw bytes, it so happens we pull it from > a String object at serialization. We do that t avoid copying it on the v8's > Serialize API side. Then, at deserialization, we end up treating it like a byte* > anyway. How about "RawBytesTag"? Done. https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2007: std::unique_ptr<const uint8_t[]>(uncompiledBytesStart), On 2016/10/12 16:01:07, Mircea Trofin wrote: > On 2016/10/12 15:05:38, jbroman wrote: > > Not new to this CL, but please don't put unowned data in an std::unique_ptr > like > > this. > > > > If it doesn't take ownership of the data, it seems to me that this is an > > indication that v8::WasmCompiledModule::UncompiledBytes should take a const > > uint8_t* rather than a std::unique_ptr<const uint8_t[]>. > > I saw the todo here, I'll update on the v8 side and re-update this CL. Thanks > for reminding me! Done. https://codereview.chromium.org/2405153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2033: CHECK(!retval.IsEmpty()); On 2016/10/12 16:01:07, Mircea Trofin wrote: > On 2016/10/12 09:34:59, jochen wrote: > > I don't think we should crash here. Downgrading is very common, e.g. ppl might > > run stable and beta in parallel. If we just can't compile the blob, we should > > throw an exception. > > Is it an exception, or is it just returning "false"? (I'm assuming the latter) > > > > > could you embed a version number in the serialized blob, so we don't even > > attempt to deserialize old, incompatible blobs? > > The serialized blob has a version number in the header. The deserialization > logic checks that first and does not go any further if the version is not a > match for the current version. This all happens on the v8 side. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. Testing for version resiliency is done on the v8 side. The Chrome side testing continues to be concerned with just data flow. Hence, no additional tests here. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. BUG= ==========
lgtm
lgtm https://codereview.chromium.org/2405153003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2405153003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1983: SerializationTag uncompiledBytesFormat = InvalidTag; wireBytesFormat?
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/19 14:27:35, titzer wrote: > lgtm > > https://codereview.chromium.org/2405153003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp > (right): > > https://codereview.chromium.org/2405153003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1983: > SerializationTag uncompiledBytesFormat = InvalidTag; > wireBytesFormat? Done - that old name was in more places, actually - fixed everywhere.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, jochen@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2405153003/#ps120001 (title: "uncompiled->wire")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. BUG= ========== to ========== [wasm] support for recompilation if deserialization fails When serializing wasm, append the uncompiled bytes explicitly. Use them when deserializing to ensure structured cloning succeeds even after chrome updates. Currently, the canonical uncompiled bytes representation is bytes - stored as a String object on the v8 side. Eventually, we want to move to a less memory-intensive option, such as Blobs. There are challenges with adopting a different representation, one being that Blobs aren't a V8 concept. In addition, we may want to explore that option in conjunction with adding support for streamed compilation, since the choice of APIs there may impact choices we make for internal representation of uncompiled bytes, and it'd be more maintainable to have one common such representation, if possible. Meanwhile, we store the payload as raw bytes, and tag that payload to make sure we can later introduce a different representation while still supporting the current one. BUG= Committed: https://crrev.com/15aaa54e5fe36c2d2bf1a53e1410be5ea274fec6 Cr-Commit-Position: refs/heads/master@{#426268} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/15aaa54e5fe36c2d2bf1a53e1410be5ea274fec6 Cr-Commit-Position: refs/heads/master@{#426268} |
