[wasm] Support wasm module structured cloning.

[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

Intent to implement: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Dogpn1hpnhw

BUG=v8:5072
Committed: https://crrev.com/e95d2b174be13defde83603e4651e703ebe91dd2
Cr-Commit-Position: refs/heads/master@{#416269}

[Mircea Trofin, 2016-08-18 02:42:22]

Patchset #1 (id:1) has been deleted

[Mircea Trofin, 2016-08-18 02:43:32]

Description was changed from

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

BUG=v8:5072
==========

to

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

BUG=v8:5072
==========

[Mircea Trofin, 2016-08-18 02:43:32]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org, jsbell@chromium.org

[Mircea Trofin, 2016-08-18 03:45:46]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 03:46:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 04:33:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 04:33:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mircea Trofin, 2016-08-18 04:51:10]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 04:51:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2016-08-18 04:51:32]

PTAL

Thanks!

[commit-bot: I haz the power, 2016-08-18 06:23:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 06:23:17]

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

[jsbell, 2016-08-18 16:21:47]

jsbell@chromium.org changed reviewers:
+ jbroman@chromium.org

[jsbell, 2016-08-18 16:21:48]

+jbroman who is reworking the serialization mechanism

(that work shouldn't impact this CL, but...)

[jsbell, 2016-08-18 16:48:02]

Quick first look (I'm on an airplane). Mostly nits but some big questions. I
didn't look at the v8 side.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_serialization_worker.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:8: if (typeof instance ===
"undefined") {
nit: why not just `instance === undefined` ?

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:14: if (typeof entrypoint !=
"function") {
nit: might as well be consistent and use !==

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:21: postMessage("didn't get
43");
Do you want a return after this postMessage too, so only one is ever sent?

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_tests.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:14: var xhrReadyState_DONE = 4;
You can rely on XMLHttpRequest.DONE being defined in Chromium. :) But...

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:21: var req = new XMLHttpRequest();
... can you use fetch() here instead of XHR since it's more readable?

fetch('incrementer.wasm')
  .then(response => {
    if (!response.ok) throw new Error(response.statusText);
    return response.arrayBuffer;
  })
  .then(data => {
    let mod = ...
    ...
  })
  .catch(error => respondToTestHarness(error.message));

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:873: }
Nit: no {} necessary for single line ifs

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1184:
v8::WasmCompiledModule::SerializedModule data = wasmModule->Serialize();
Is this a copy, or does SerializedModule point into the existing bytes? If it's
a copy, can we know the size before we call Serialize(), and change Serialize()
to take a writer?

(that could be follow-up work to avoid the copy if necessary and possible)

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1858:
std::unique_ptr<const uint8_t[]>(buf),
Uh... it seems like the SerializedModule constructor should not take a
unique_ptr if this is safe. If it's not claiming ownership of the data (i.e.
doing a std::move on the ptr) why is it taking a unique_ptr in the first place?

[jbroman, 2016-08-18 16:59:09]

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
v8::WasmCompiledModule::Deserialize(isolate(), data);
If I read this correctly, every time the V8 version changes every compiled
module stored in IndexedDB will become invalid, and any attempt to read a record
containing a module will fail (IIRC this appears as the object being
deserialized as null).

https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a...
doesn't mention this behaviour.

[Mircea Trofin, 2016-08-18 17:34:08]

Thanks for the feedback so far, inline some answers. I'll wait for more feedback
before updating the CL.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_serialization_worker.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:8: if (typeof instance ===
"undefined") {
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> nit: why not just `instance === undefined` ?

Acknowledged.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:14: if (typeof entrypoint !=
"function") {
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> nit: might as well be consistent and use !==

Acknowledged.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:21: postMessage("didn't get
43");
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> Do you want a return after this postMessage too, so only one is ever sent?

Acknowledged.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_tests.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:14: var xhrReadyState_DONE = 4;
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> You can rely on XMLHttpRequest.DONE being defined in Chromium. :) But...

Oh! Nice!

why the "but"? Would we run these tests elsewhere? (honest question, not
familiar with chromium policies)

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:21: var req = new XMLHttpRequest();
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> ... can you use fetch() here instead of XHR since it's more readable?
> 
> fetch('incrementer.wasm')
>   .then(response => {
>     if (!response.ok) throw new Error(response.statusText);
>     return response.arrayBuffer;
>   })
>   .then(data => {
>     let mod = ...
>     ...
>   })
>   .catch(error => respondToTestHarness(error.message));
> 

Acknowledged.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:873: }
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> Nit: no {} necessary for single line ifs

Acknowledged.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1184:
v8::WasmCompiledModule::SerializedModule data = wasmModule->Serialize();
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> Is this a copy, or does SerializedModule point into the existing bytes? If
it's
> a copy, can we know the size before we call Serialize(), and change
Serialize()
> to take a writer?
> 
> (that could be follow-up work to avoid the copy if necessary and possible)

SerializedModule points to existing bytes, at the moment.

I am overall concerned with the need to copy bytes. I would prefer what you
suggest - taking some form of writer, and, in addition, avoiding the need to
allocate contiguous memory for the data. Wasm compiled modules may be assumed to
be in the order of tens of megs, at least, so I'm concerned both with copying
and with the memory pressure the contiguous memory requirement creates. On that,
is segmented writing something currently supported?

I was planning to look into those details as part of next steps. My first goal
was to ensure the end to end story holds up. Any pushback to staging it this
way?

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1858:
std::unique_ptr<const uint8_t[]>(buf),
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> Uh... it seems like the SerializedModule constructor should not take a
> unique_ptr if this is safe. If it's not claiming ownership of the data (i.e.
> doing a std::move on the ptr) why is it taking a unique_ptr in the first
place?

SerializedModule is just a pair used by both serializer and deserializer. The
idea was to enforce the contract that 
1) the deserializer doesn't own the data, and shouldn't delete it. Passing const
uint8_t* would't be enough, one can still delete that. A const std::unique_ptr&,
however, makes that trickier.
2) the serializer relinquishes the control of the data to the caller (hence the
unique_ptr)

But maybe that's too overboard. We could just pass to the deserializer the const
uint8_t* and the length and document the ownership aspect. The serializer and
deserializer wouldn't have symmetric APIs, but that's not a strong argument,
imho, especially when I doubt any symmetry would be left once we have a
non-copying, segmented design.

...and maybe there's another option?

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
v8::WasmCompiledModule::Deserialize(isolate(), data);
On 2016/08/18 16:59:09, jbroman wrote:
> If I read this correctly, every time the V8 version changes every compiled
> module stored in IndexedDB will become invalid, and any attempt to read a
record
> containing a module will fail (IIRC this appears as the object being
> deserialized as null).
> 
>
https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a...
> doesn't mention this behaviour.

That's correct, the intent is to not support deserialization across v8 versions.
Returning undefined is one option, throwing is another one (there may be more).
The spec leaves this detail unspecified (I'll follow up on that), and we still
need to figure out, for IndexedDB, what would happen with the stored bits in
this case. So there are a few loose ends still left to be tied, but the goal of
this CL is to be a first step and make sure the basic end to end plumbing works
as expected.

Does this staging approach for enabling the feature make sense?

[jbroman, 2016-08-18 17:55:50]

FYI, as jsbell mentioned, I'm in the process of moving the core of
ScriptValueSerializer into V8, so (once I get past the basic objects) I will
likely end up refactoring much of this class. (Just so you aren't surprised when
this code starts moving later this quarter.)

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
v8::WasmCompiledModule::Deserialize(isolate(), data);
On 2016/08/18 at 17:34:08, Mircea Trofin wrote:
> On 2016/08/18 16:59:09, jbroman wrote:
> > If I read this correctly, every time the V8 version changes every compiled
> > module stored in IndexedDB will become invalid, and any attempt to read a
record
> > containing a module will fail (IIRC this appears as the object being
> > deserialized as null).
> > 
> >
https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a...
> > doesn't mention this behaviour.
> 
> That's correct, the intent is to not support deserialization across v8
versions. Returning undefined is one option, throwing is another one (there may
be more). The spec leaves this detail unspecified (I'll follow up on that), and
we still need to figure out, for IndexedDB, what would happen with the stored
bits in this case. So there are a few loose ends still left to be tied, but the
goal of this CL is to be a first step and make sure the basic end to end
plumbing works as expected.
> 
> Does this staging approach for enabling the feature make sense?

Seems fine, as long as this is addressed before shipping.

[jsbell, 2016-08-18 18:13:14]

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_tests.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:14: var xhrReadyState_DONE = 4;
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> why the "but"?

Just that if you replace XHR with fetch() you don't need DONE. Apologies for
being vague.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1184:
v8::WasmCompiledModule::SerializedModule data = wasmModule->Serialize();
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> I was planning to look into those details as part of next steps. My first goal
> was to ensure the end to end story holds up. Any pushback to staging it this
> way?

Staging is great! (I'm a fan of leaving TODOs in the code in that case)

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1858:
std::unique_ptr<const uint8_t[]>(buf),
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> But maybe that's too overboard. We could just pass to the deserializer the
const
> uint8_t* and the length and document the ownership aspect. The serializer and
> deserializer wouldn't have symmetric APIs, but that's not a strong argument,
> imho, especially when I doubt any symmetry would be left once we have a
> non-copying, segmented design.

I'd start with this, pending any better ideas.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
v8::WasmCompiledModule::Deserialize(isolate(), data);
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> Returning undefined is one option, throwing is another one (there may be
more).
> The spec leaves this detail unspecified (I'll follow up on that), and we still
> need to figure out, for IndexedDB, what would happen with the stored bits in
> this case.

I think what we settled on (in email conversations) is that we'd yield a Module
that was "neutered" in the same way as a closed Blob - an object makes it out of
the database but produces errors if you try and use it. But that may not have
made it into the spec.

> Does this staging approach for enabling the feature make sense?

+1 to staging. (and sprinkles of TODOs that link to tracking crbugs would be
awesome!)

[Mircea Trofin, 2016-08-18 18:14:01]

On 2016/08/18 17:55:50, jbroman wrote:
> FYI, as jsbell mentioned, I'm in the process of moving the core of
> ScriptValueSerializer into V8, so (once I get past the basic objects) I will
> likely end up refactoring much of this class. (Just so you aren't surprised
when
> this code starts moving later this quarter.)
> 
> 
Sounds good - any plans for supporting non-copying, segmented serialization?

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
> (right):
> 
>
https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
> v8::WasmCompiledModule::Deserialize(isolate(), data);
> On 2016/08/18 at 17:34:08, Mircea Trofin wrote:
> > On 2016/08/18 16:59:09, jbroman wrote:
> > > If I read this correctly, every time the V8 version changes every compiled
> > > module stored in IndexedDB will become invalid, and any attempt to read a
> record
> > > containing a module will fail (IIRC this appears as the object being
> > > deserialized as null).
> > > 
> > >
>
https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a...
> > > doesn't mention this behaviour.
> > 
> > That's correct, the intent is to not support deserialization across v8
> versions. Returning undefined is one option, throwing is another one (there
may
> be more). The spec leaves this detail unspecified (I'll follow up on that),
and
> we still need to figure out, for IndexedDB, what would happen with the stored
> bits in this case. So there are a few loose ends still left to be tied, but
the
> goal of this CL is to be a first step and make sure the basic end to end
> plumbing works as expected.
> > 
> > Does this staging approach for enabling the feature make sense?
> 
> Seems fine, as long as this is addressed before shipping.

I agree - if by shipping we mean "when we get wasm off from being behind a flag"

[jbroman, 2016-08-18 18:18:23]

On 2016/08/18 at 18:14:01, mtrofin wrote:
> On 2016/08/18 17:55:50, jbroman wrote:
> > FYI, as jsbell mentioned, I'm in the process of moving the core of
> > ScriptValueSerializer into V8, so (once I get past the basic objects) I will
> > likely end up refactoring much of this class. (Just so you aren't surprised
when
> > this code starts moving later this quarter.)
> > 
> > 
> Sounds good - any plans for supporting non-copying, segmented serialization?

Not at present, sorry.

>
https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
> > (right):
> > 
> >
https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
> > v8::WasmCompiledModule::Deserialize(isolate(), data);
> > On 2016/08/18 at 17:34:08, Mircea Trofin wrote:
> > > On 2016/08/18 16:59:09, jbroman wrote:
> > > > If I read this correctly, every time the V8 version changes every
compiled
> > > > module stored in IndexedDB will become invalid, and any attempt to read
a
> > record
> > > > containing a module will fail (IIRC this appears as the object being
> > > > deserialized as null).
> > > > 
> > > >
> >
https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a...
> > > > doesn't mention this behaviour.
> > > 
> > > That's correct, the intent is to not support deserialization across v8
> > versions. Returning undefined is one option, throwing is another one (there
may
> > be more). The spec leaves this detail unspecified (I'll follow up on that),
and
> > we still need to figure out, for IndexedDB, what would happen with the
stored
> > bits in this case. So there are a few loose ends still left to be tied, but
the
> > goal of this CL is to be a first step and make sure the basic end to end
> > plumbing works as expected.
> > > 
> > > Does this staging approach for enabling the feature make sense?
> > 
> > Seems fine, as long as this is addressed before shipping.
> 
> I agree - if by shipping we mean "when we get wasm off from being behind a
flag"

Yeah, to me "shipping" means "when a user using Chrome without special flags
could run the code".

[Mircea Trofin, 2016-08-18 21:53:15]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 21:53:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2016-08-18 23:02:59]

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_serialization_worker.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:8: if (typeof instance ===
"undefined") {
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> > nit: why not just `instance === undefined` ?
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:14: if (typeof entrypoint !=
"function") {
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> > nit: might as well be consistent and use !==
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_serialization_worker.js:21: postMessage("didn't get
43");
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> > Do you want a return after this postMessage too, so only one is ever sent?
> 
> Acknowledged.

Actually, don't need to post that string, can just return. This was a remnant
from earlier code.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
File chrome/test/data/wasm/wasm_tests.js (right):

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:14: var xhrReadyState_DONE = 4;
On 2016/08/18 18:13:14, jsbell (slow thru Aug 18) wrote:
> On 2016/08/18 17:34:08, Mircea Trofin wrote:
> > why the "but"?
> 
> Just that if you replace XHR with fetch() you don't need DONE. Apologies for
> being vague.

Done.

https://codereview.chromium.org/2255673003/diff/60001/chrome/test/data/wasm/w...
chrome/test/data/wasm/wasm_tests.js:21: var req = new XMLHttpRequest();
On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> ... can you use fetch() here instead of XHR since it's more readable?
> 
> fetch('incrementer.wasm')
>   .then(response => {
>     if (!response.ok) throw new Error(response.statusText);
>     return response.arrayBuffer;
>   })
>   .then(data => {
>     let mod = ...
>     ...
>   })
>   .catch(error => respondToTestHarness(error.message));
> 

Done.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:873: }
On 2016/08/18 17:34:08, Mircea Trofin wrote:
> On 2016/08/18 16:48:02, jsbell (slow thru Aug 18) wrote:
> > Nit: no {} necessary for single line ifs
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1184:
v8::WasmCompiledModule::SerializedModule data = wasmModule->Serialize();
On 2016/08/18 18:13:14, jsbell (slow thru Aug 18) wrote:
> On 2016/08/18 17:34:08, Mircea Trofin wrote:
> > I was planning to look into those details as part of next steps. My first
goal
> > was to ensure the end to end story holds up. Any pushback to staging it this
> > way?
> 
> Staging is great! (I'm a fan of leaving TODOs in the code in that case)

Done.

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1858:
std::unique_ptr<const uint8_t[]>(buf),
On 2016/08/18 18:13:14, jsbell (slow thru Aug 18) wrote:
> On 2016/08/18 17:34:08, Mircea Trofin wrote:
> > But maybe that's too overboard. We could just pass to the deserializer the
> const
> > uint8_t* and the length and document the ownership aspect. The serializer
and
> > deserializer wouldn't have symmetric APIs, but that's not a strong argument,
> > imho, especially when I doubt any symmetry would be left once we have a
> > non-copying, segmented design.
> 
> I'd start with this, pending any better ideas.

Added a TODO, and will follow with the v8 CL + chrome CL subsequently

https://codereview.chromium.org/2255673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1862:
v8::WasmCompiledModule::Deserialize(isolate(), data);
On 2016/08/18 18:13:14, jsbell (slow thru Aug 18) wrote:
> On 2016/08/18 17:34:08, Mircea Trofin wrote:
> > Returning undefined is one option, throwing is another one (there may be
> more).
> > The spec leaves this detail unspecified (I'll follow up on that), and we
still
> > need to figure out, for IndexedDB, what would happen with the stored bits in
> > this case.
> 
> I think what we settled on (in email conversations) is that we'd yield a
Module
> that was "neutered" in the same way as a closed Blob - an object makes it out
of
> the database but produces errors if you try and use it. But that may not have
> made it into the spec.
> 
> > Does this staging approach for enabling the feature make sense?
> 
> +1 to staging. (and sprinkles of TODOs that link to tracking crbugs would be
> awesome!)

Done.

[commit-bot: I haz the power, 2016-08-18 23:26:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 23:26:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mircea Trofin, 2016-08-19 04:01:02]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 04:01:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 04:03:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 04:03:59]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mircea Trofin, 2016-08-19 04:17:48]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 04:18:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2016-08-19 04:20:29]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-08-19 05:26:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 05:26:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mircea Trofin, 2016-08-20 05:11:24]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 05:11:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 05:13:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-20 05:13:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mircea Trofin, 2016-08-20 05:26:52]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 05:27:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 07:06:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-20 07:06:40]

Dry run: Exceeded global retry quota

[Mircea Trofin, 2016-08-20 19:10:11]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 19:10:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 20:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-20 20:09:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mircea Trofin, 2016-08-20 21:54:59]

PTAL - does the isolate setup make sense, for copying over the wasm
dependency?

Is there anything else I should address at this point?

Thanks!

[Mircea Trofin, 2016-08-23 15:08:21]

On 2016/08/20 21:54:59, Mircea Trofin wrote:
> PTAL - does the isolate setup make sense, for copying over the wasm
> dependency?
> 
> Is there anything else I should address at this point?
> 
> Thanks!

Gentle nudge.

Thanks!

[jbroman, 2016-08-23 15:23:06]

jbroman@chromium.org changed reviewers:
- jbroman@chromium.org

[jbroman, 2016-08-23 15:23:07]

(moving self to cc, as I think it's the other reviewers you're waiting for?)

https://codereview.chromium.org/2255673003/diff/180001/DEPS
File DEPS (right):

https://codereview.chromium.org/2255673003/diff/180001/DEPS#newcode43
DEPS:43: 'v8_revision': '93b7251f7482246e9f57ae118295452f4b38dd75',
Rolling v8 in this CL seems strange.

[Mircea Trofin, 2016-08-23 15:48:58]

On 2016/08/23 15:23:07, jbroman wrote:
> (moving self to cc, as I think it's the other reviewers you're waiting for?)
> 
Oh... true. +jochen

> https://codereview.chromium.org/2255673003/diff/180001/DEPS
> File DEPS (right):
> 
> https://codereview.chromium.org/2255673003/diff/180001/DEPS#newcode43
> DEPS:43: 'v8_revision': '93b7251f7482246e9f57ae118295452f4b38dd75',
> Rolling v8 in this CL seems strange.

What's the alternative, since the change on the chrome side requires the APIs 
made available in v8 at that rev?

[Mircea Trofin, 2016-08-23 15:49:24]

mtrofin@chromium.org changed reviewers:
+ jochen@chromium.org

[Mircea Trofin, 2016-08-23 15:49:56]

Jochen, could you please take a look at this? Thanks!

[jbroman, 2016-08-23 17:15:10]

On 2016/08/23 at 15:48:58, mtrofin wrote:
> On 2016/08/23 15:23:07, jbroman wrote:
> > (moving self to cc, as I think it's the other reviewers you're waiting for?)
> > 
> Oh... true. +jochen
> 
> > https://codereview.chromium.org/2255673003/diff/180001/DEPS
> > File DEPS (right):
> > 
> > https://codereview.chromium.org/2255673003/diff/180001/DEPS#newcode43
> > DEPS:43: 'v8_revision': '93b7251f7482246e9f57ae118295452f4b38dd75',
> > Rolling v8 in this CL seems strange.
> 
> What's the alternative, since the change on the chrome side requires the APIs 
> made available in v8 at that rev?

There's a bot that automatically advances v8_revision:
https://chromium.googlesource.com/chromium/src/+log/master?author=v8-autoroll

You just have to wait for it to advance past your change (which it already has).

[blink-reviews, 2016-08-23 17:19:24]

Oh, awesome. Thanks!
On Tue, Aug 23, 2016 at 10:15 AM <jbroman@chromium.org> wrote:

> On 2016/08/23 at 15:48:58, mtrofin wrote:
> > On 2016/08/23 15:23:07, jbroman wrote:
> > > (moving self to cc, as I think it's the other reviewers you're waiting
> for?)
> > >
> > Oh... true. +jochen
> >
> > > https://codereview.chromium.org/2255673003/diff/180001/DEPS
> > > File DEPS (right):
> > >
> > > https://codereview.chromium.org/2255673003/diff/180001/DEPS#newcode43
> > > DEPS:43: 'v8_revision': '93b7251f7482246e9f57ae118295452f4b38dd75',
> > > Rolling v8 in this CL seems strange.
> >
> > What's the alternative, since the change on the chrome side requires the
> APIs
> > made available in v8 at that rev?
>
> There's a bot that automatically advances v8_revision:
>
> https://chromium.googlesource.com/chromium/src/+log/master?author=v8-autoroll
>
> You just have to wait for it to advance past your change (which it already
> has).
>
> https://codereview.chromium.org/2255673003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-08-23 17:19:35]

Oh, awesome. Thanks!
On Tue, Aug 23, 2016 at 10:15 AM <jbroman@chromium.org> wrote:

> On 2016/08/23 at 15:48:58, mtrofin wrote:
> > On 2016/08/23 15:23:07, jbroman wrote:
> > > (moving self to cc, as I think it's the other reviewers you're waiting
> for?)
> > >
> > Oh... true. +jochen
> >
> > > https://codereview.chromium.org/2255673003/diff/180001/DEPS
> > > File DEPS (right):
> > >
> > > https://codereview.chromium.org/2255673003/diff/180001/DEPS#newcode43
> > > DEPS:43: 'v8_revision': '93b7251f7482246e9f57ae118295452f4b38dd75',
> > > Rolling v8 in this CL seems strange.
> >
> > What's the alternative, since the change on the chrome side requires the
> APIs
> > made available in v8 at that rev?
>
> There's a bot that automatically advances v8_revision:
>
> https://chromium.googlesource.com/chromium/src/+log/master?author=v8-autoroll
>
> You just have to wait for it to advance past your change (which it already
> has).
>
> https://codereview.chromium.org/2255673003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mircea Trofin, 2016-08-23 17:56:24]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 17:57:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 19:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 19:46:08]

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

[jochen (gone - plz use gerrit), 2016-08-24 14:25:39]

why didn't you write the test as layout test?

this allows for sharing binary code between origins / processes - has this been
run by the security team?

[Mircea Trofin, 2016-08-24 14:51:34]

mtrofin@chromium.org changed reviewers:
+ jschuh@chromium.org

[Mircea Trofin, 2016-08-24 14:56:42]

Justin, could you please take a look at this CL - see 
Jochen's comment.

Would attaching the origin to the serialized stream be 
sufficient to address security concerns?

Thanks!

[jochen (gone - plz use gerrit), 2016-08-26 15:14:02]

can you add an explicit runtime enabled feature flag to control this feature?

[Mircea Trofin, 2016-08-29 23:59:02]

On 2016/08/26 15:14:02, jochen wrote:
> can you add an explicit runtime enabled feature flag to control this feature?

Added.

The last outstanding issue, to my knowledge, is LayoutTests: it turns out we
can't currently
pick files outside the LayoutTests directory. I'd prefer not copying the wasm
file there, as
to avoid the churn of keeping it in sync with v8 changes. Any pushback to
keeping tests 
for this feature as browser_tests, at least for the time being?

Thanks!

[jochen (gone - plz use gerrit), 2016-08-30 12:14:28]

if it's a browser test, it should be a content_browsertest

[Mircea Trofin, 2016-08-31 21:57:13]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 21:57:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 01:04:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 01:04:26]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-09-01 01:06:15]

PTAL

Based on offline feedback, moved the wasm file under LayoutTests, and
testing is now done via that (==LayoutTests) framework.

[jochen (gone - plz use gerrit), 2016-09-01 12:13:59]

code looks good. Now all that's missing is the intent to implement email..

https://codereview.chromium.org/2255673003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2255673003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:229:
WebAssemblySerialization status=experimental
let's keep test as status = test until the origin restrictions are implemented

[azizyahyaxx, 2016-09-01 18:01:30]

On 2016/09/01 12:13:59, jochen wrote:
> code looks good. Now all that's missing is the intent to implement email..
> 
>
https://codereview.chromium.org/2255673003/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/2255673003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:229:
> WebAssemblySerialization status=experimental
> let's keep test as status = test until the origin restrictions are implemented

Azizfida

[Mircea Trofin, 2016-09-01 18:15:19]

PTAL. Thanks!

[Mircea Trofin, 2016-09-02 07:14:59]

https://codereview.chromium.org/2255673003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2255673003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:229:
WebAssemblySerialization status=experimental
On 2016/09/01 12:13:59, jochen wrote:
> let's keep test as status = test until the origin restrictions are implemented

Done.

[jochen (gone - plz use gerrit), 2016-09-02 12:33:29]

code looks good.

Once you've send the intent to implement, please link to it from the CL
description, and I will approve

[Mircea Trofin, 2016-09-02 14:00:37]

Description was changed from

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

BUG=v8:5072
==========

to

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Dogpn1hpnhw

BUG=v8:5072
==========

[jochen (gone - plz use gerrit), 2016-09-02 14:01:25]

lgtm

[Mircea Trofin, 2016-09-02 14:01:52]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-09-02 14:02:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 15:33:01]

Description was changed from

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Dogpn1hpnhw

BUG=v8:5072
==========

to

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Dogpn1hpnhw

BUG=v8:5072
==========

[commit-bot: I haz the power, 2016-09-02 15:33:02]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-09-02 15:36:18]

Description was changed from

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Dogpn1hpnhw

BUG=v8:5072
==========

to

==========
[wasm] Support wasm module structured cloning.

Wasm modules may be large (megs) and their compilation is time- and
resources consuming (many seconds on desktops, more on mobile).

With structured cloning support, developers may save/restore compilation
results in IndexedDB, or decide to compile on worker threads.

This first CL introduces basic support and a basic test.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Dogpn1hpnhw

BUG=v8:5072

Committed: https://crrev.com/e95d2b174be13defde83603e4651e703ebe91dd2
Cr-Commit-Position: refs/heads/master@{#416269}
==========

[commit-bot: I haz the power, 2016-09-02 15:36:20]

Patchset 12 (id:??) landed as
https://crrev.com/e95d2b174be13defde83603e4651e703ebe91dd2
Cr-Commit-Position: refs/heads/master@{#416269}