[wasm] Avoid double-serializing the wire bytes

[wasm] Avoid double-serializing the wire bytes

Since the public API for deserialization is now just DeserializeOrCompile,
we can trickle down the wire bytes to the deserialization logic, and
avoid the need for duplicating the wire bytes when serializing.

BUG=chromium:657316
Committed: https://crrev.com/91a5a219d46d592b49f6e963d308b2bbdc9b5b60
Cr-Commit-Position: refs/heads/master@{#40516}

[Mircea Trofin, 2016-10-20 05:46:25]

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

[commit-bot: I haz the power, 2016-10-20 05:46:27]

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-10-20 05:51:26]

Description was changed from

==========
[wasm] Avoid double-serializing the wire bytes

BUG=
==========

to

==========
[wasm] Avoid double-serializing the wire bytes

Since the public API for deserialization is now just DeserializeOrCompile,
we can trickle down the wire bytes to the deserialization logic, and 
avoid the need for duplicating the wire bytes when serializing.

BUG=chromium:657316
==========

[Mircea Trofin, 2016-10-20 05:51:26]

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

[commit-bot: I haz the power, 2016-10-20 06:27:22]

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

[commit-bot: I haz the power, 2016-10-20 06:27:23]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-10-20 06:46:17]

yangguo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Yang, 2016-10-20 06:46:17]

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/runtime/runtime-...
File src/runtime/runtime-test.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/runtime/runtime-...
src/runtime/runtime-test.cc:770:
{reinterpret_cast<uint8_t*>(wire_bytes->backing_store()),
I think it helps with readability if you use the Vector constructor explicitly
here.

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/snapshot/code-se...
File src/snapshot/code-serializer.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/snapshot/code-se...
src/snapshot/code-serializer.cc:226: Handle<SeqOneByteString> wire_bytes =
compiled_module->module_bytes();
Another way to do this is to use attached references. You would register this
string as attached reference so that the serializer recognizes this object. The
deserializer then restores it. This is what we do for the source string in
CodeSerializer.

This way, if there are other references to wire bytes in the object graph, the
serializer would find them. Which may not be the case now, but maybe sometime in
the future?

[Mircea Trofin, 2016-10-20 07:18:18]

Patchset #2 (id:20001) has been deleted

[Mircea Trofin, 2016-10-20 07:22:07]

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

[commit-bot: I haz the power, 2016-10-20 07:22:15]

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-10-20 07:22:30]

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/runtime/runtime-...
File src/runtime/runtime-test.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/runtime/runtime-...
src/runtime/runtime-test.cc:770:
{reinterpret_cast<uint8_t*>(wire_bytes->backing_store()),
On 2016/10/20 06:46:17, Yang wrote:
> I think it helps with readability if you use the Vector constructor explicitly
> here.

Done.

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/snapshot/code-se...
File src/snapshot/code-serializer.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/1/src/snapshot/code-se...
src/snapshot/code-serializer.cc:226: Handle<SeqOneByteString> wire_bytes =
compiled_module->module_bytes();
On 2016/10/20 06:46:17, Yang wrote:
> Another way to do this is to use attached references. You would register this
> string as attached reference so that the serializer recognizes this object.
The
> deserializer then restores it. This is what we do for the source string in
> CodeSerializer.
> 
> This way, if there are other references to wire bytes in the object graph, the
> serializer would find them. Which may not be the case now, but maybe sometime
in
> the future?

Oh... yes. Yes, makes sense :) Thanks!

[Mircea Trofin, 2016-10-20 07:25:01]

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

[commit-bot: I haz the power, 2016-10-20 07:25:05]

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-10-20 08:02:34]

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

[commit-bot: I haz the power, 2016-10-20 08:02:35]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-20 08:32:08]

https://chromiumcodereview.appspot.com/2433273002/diff/60001/src/runtime/runt...
File src/runtime/runtime-test.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/60001/src/runtime/runt...
src/runtime/runtime-test.cc:771:
reinterpret_cast<uint8_t*>(wire_bytes->backing_store()),
This raw pointer is unsafe unless the buffer is externalized first.

[Mircea Trofin, 2016-10-20 15:58:33]

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

[commit-bot: I haz the power, 2016-10-20 15:58:41]

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-10-20 16:04:30]

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

[Mircea Trofin, 2016-10-20 16:06:42]

Michael,  could you please check my use of the ArrayBuffer Register/Unregister
APIs (see runtime-test.cc)? Is that how they are intended to be used, and, if
not, what is the recommended pattern (save memcpy-ing to a native buffer, which
would be so not elegant :) )

Thanks!

https://chromiumcodereview.appspot.com/2433273002/diff/60001/src/runtime/runt...
File src/runtime/runtime-test.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/60001/src/runtime/runt...
src/runtime/runtime-test.cc:771:
reinterpret_cast<uint8_t*>(wire_bytes->backing_store()),
On 2016/10/20 08:32:08, titzer wrote:
> This raw pointer is unsafe unless the buffer is externalized first.

Done, and added mlippautz to double-check this is the expected use of the
Register/Unregister APIs.

[commit-bot: I haz the power, 2016-10-20 16:19:18]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11226)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Michael Lippautz, 2016-10-20 16:20:31]

The usage is (almost) correct and you will not create a copy for the
unregister/register dance.

Didn't look at the rest.

https://codereview.chromium.org/2433273002/diff/80001/src/runtime/runtime-tes...
File src/runtime/runtime-test.cc (right):

https://codereview.chromium.org/2433273002/diff/80001/src/runtime/runtime-tes...
src/runtime/runtime-test.cc:780: wire_bytes->set_is_external(false);
Please move "set_is_external(false)" above the registration call. We might add a
check at some point that we don't register external array buffers (which would
fire in this case).

[titzer, 2016-10-20 16:25:16]

On 2016/10/20 16:20:31, Michael Lippautz wrote:
> The usage is (almost) correct and you will not create a copy for the
> unregister/register dance.
> 
> Didn't look at the rest.
> 
>
https://codereview.chromium.org/2433273002/diff/80001/src/runtime/runtime-tes...
> File src/runtime/runtime-test.cc (right):
> 
>
https://codereview.chromium.org/2433273002/diff/80001/src/runtime/runtime-tes...
> src/runtime/runtime-test.cc:780: wire_bytes->set_is_external(false);
> Please move "set_is_external(false)" above the registration call. We might add
a
> check at some point that we don't register external array buffers (which would
> fire in this case).

Thanks, Michael.

Can we also leave a TODO here to clean this API up? I think I've seen code like
this a few times before.

[Mircea Trofin, 2016-10-20 16:31:26]

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

[commit-bot: I haz the power, 2016-10-20 16:31:28]

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-10-20 16:32:52]

On 2016/10/20 16:25:16, titzer wrote:
> On 2016/10/20 16:20:31, Michael Lippautz wrote:
> > The usage is (almost) correct and you will not create a copy for the
> > unregister/register dance.
> > 
> > Didn't look at the rest.
> > 
> >
>
https://codereview.chromium.org/2433273002/diff/80001/src/runtime/runtime-tes...
> > File src/runtime/runtime-test.cc (right):
> > 
> >
>
https://codereview.chromium.org/2433273002/diff/80001/src/runtime/runtime-tes...
> > src/runtime/runtime-test.cc:780: wire_bytes->set_is_external(false);
> > Please move "set_is_external(false)" above the registration call. We might
add
> a
> > check at some point that we don't register external array buffers (which
would
> > fire in this case).
> 
> Thanks, Michael.
> 
> Can we also leave a TODO here to clean this API up? I think I've seen code
like
> this a few times before.

Added a TODO on the Register/Unregister declaration side of things.

[Mircea Trofin, 2016-10-20 16:33:42]

https://chromiumcodereview.appspot.com/2433273002/diff/80001/src/runtime/runt...
File src/runtime/runtime-test.cc (right):

https://chromiumcodereview.appspot.com/2433273002/diff/80001/src/runtime/runt...
src/runtime/runtime-test.cc:780: wire_bytes->set_is_external(false);
On 2016/10/20 16:20:31, Michael Lippautz wrote:
> Please move "set_is_external(false)" above the registration call. We might add
a
> check at some point that we don't register external array buffers (which would
> fire in this case).

Done.

[commit-bot: I haz the power, 2016-10-20 17:04:08]

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

[commit-bot: I haz the power, 2016-10-20 17:04:09]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-10-20 18:09:15]

mtrofin@chromium.org changed reviewers:
+ verwaest@chromium.org, vogelheim@chromium.org

[Mircea Trofin, 2016-10-20 18:10:42]

PTAL.

I need additional signoff from snaphshot folks, hence adding respective owners.

Thanks!

[Michael Lippautz, 2016-10-20 19:20:46]

heap/ lgtm

although the methods on heap will likely never internalize/externalize for you
but merely DCHECK !externalized. The methods you want should go on
JSArrayBuffer.

[Yang, 2016-10-21 05:21:43]

On 2016/10/20 19:20:46, Michael Lippautz wrote:
> heap/ lgtm
> 
> although the methods on heap will likely never internalize/externalize for you
> but merely DCHECK !externalized. The methods you want should go on
> JSArrayBuffer.

src/snapshot lgtm.

[Mircea Trofin, 2016-10-21 17:41:37]

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

[commit-bot: I haz the power, 2016-10-21 17:41:40]

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-10-21 17:46:10]

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

[commit-bot: I haz the power, 2016-10-21 17:46:11]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11314)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16576)

[Mircea Trofin, 2016-10-21 17:50:17]

Patchset #7 (id:140001) has been deleted

[Mircea Trofin, 2016-10-21 17:50:41]

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

[commit-bot: I haz the power, 2016-10-21 17:50:50]

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-10-21 18:04:24]

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

[Mircea Trofin, 2016-10-21 18:04:30]

Patchset #7 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-10-21 18:04:34]

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-10-21 18:20:08]

ptal - removed unrelated changes (thanks, Ben, for noticing!)

[commit-bot: I haz the power, 2016-10-21 18:35:57]

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

[commit-bot: I haz the power, 2016-10-21 18:35:58]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-22 14:43:04]

On 2016/10/21 18:20:08, Mircea Trofin wrote:
> ptal - removed unrelated changes (thanks, Ben, for noticing!)

lgtm

[Mircea Trofin, 2016-10-22 15:12:00]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-10-22 15:12:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/2433273002/#ps180001
(title: "removed unrelated changes")

[commit-bot: I haz the power, 2016-10-22 15:12:09]

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-10-22 15:14:48]

Description was changed from

==========
[wasm] Avoid double-serializing the wire bytes

Since the public API for deserialization is now just DeserializeOrCompile,
we can trickle down the wire bytes to the deserialization logic, and 
avoid the need for duplicating the wire bytes when serializing.

BUG=chromium:657316
==========

to

==========
[wasm] Avoid double-serializing the wire bytes

Since the public API for deserialization is now just DeserializeOrCompile,
we can trickle down the wire bytes to the deserialization logic, and 
avoid the need for duplicating the wire bytes when serializing.

BUG=chromium:657316
==========

[commit-bot: I haz the power, 2016-10-22 15:14:50]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-11-17 22:10:21]

Description was changed from

==========
[wasm] Avoid double-serializing the wire bytes

Since the public API for deserialization is now just DeserializeOrCompile,
we can trickle down the wire bytes to the deserialization logic, and 
avoid the need for duplicating the wire bytes when serializing.

BUG=chromium:657316
==========

to

==========
[wasm] Avoid double-serializing the wire bytes

Since the public API for deserialization is now just DeserializeOrCompile,
we can trickle down the wire bytes to the deserialization logic, and
avoid the need for duplicating the wire bytes when serializing.

BUG=chromium:657316

Committed: https://crrev.com/91a5a219d46d592b49f6e963d308b2bbdc9b5b60
Cr-Commit-Position: refs/heads/master@{#40516}
==========

[commit-bot: I haz the power, 2016-11-17 22:10:22]

Patchset 7 (id:??) landed as
https://crrev.com/91a5a219d46d592b49f6e963d308b2bbdc9b5b60
Cr-Commit-Position: refs/heads/master@{#40516}