[wasm] Serialization/Deserialization of compiled module

[wasm] Serialization/Deserialization of compiled module

Implementation of serialization/deserialization for compiled wasm
module.

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

[Mircea Trofin, 2016-08-04 15:56:26]

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

[commit-bot: I haz the power, 2016-08-04 15:56:35]

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-04 15:59:01]

Description was changed from

==========
Serialization

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

to

==========
[wasm] Serialization/Deserialization of compiled module

Implementation of serialization/deserialization for compiled wasm 
module. 

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

[Mircea Trofin, 2016-08-04 15:59:02]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org, rossberg@chromium.org, yangguo@chromium.org

[Mircea Trofin, 2016-08-04 16:06:40]

Yang, PTAL - I was able to leverage the current serialization 
infrastructure with minimal changes - generalizing a bit existing 
functionality. Does it make sense?

Andreas, could you please take a look at the wasm side changes for the 
setup of the compiled module? We can't rely on the original source 
being present because that won't be available at deserialization. Also,
avoiding keeping it alive may be "a good thing", to reduce memory 
pressure.

A second question: what should be the valid relationships between 2 
compilations of the same wasm buffer (which, by extension, I'd assume
to need to hold for a compiled module and its deserialization)?

[commit-bot: I haz the power, 2016-08-04 16:27:19]

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

[commit-bot: I haz the power, 2016-08-04 16:27:20]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10333)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Mircea Trofin, 2016-08-04 16:38:09]

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

[commit-bot: I haz the power, 2016-08-04 16:38:19]

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-04 16:57:49]

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

[commit-bot: I haz the power, 2016-08-04 16:57:49]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-08-04 18:35:19]

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

[Yang, 2016-08-05 12:04:18]

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.cc:98: SerializeGeneric(code_object, how_to_code,
where_to_point);
Would it make sense to check for code_object->has_reloc_info_for_serialization()
here as well?

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.h (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.h:50: enum SanityCheckResult {
Can we keep this in SerializedCodeData? We can make it public.

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1530:
cs.reference_map()->AddAttachedReference(*isolate->native_context());
Can we put all this into src/snapshot in a new subclass of CodeSerializer? Aside
from CreateCompiledModuleObject, none of this contains anything wasm-specific.

[Yang, 2016-08-05 12:05:44]

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.cc:98: SerializeGeneric(code_object, how_to_code,
where_to_point);
On 2016/08/05 12:04:17, Yang wrote:
> Would it make sense to check for
code_object->has_reloc_info_for_serialization()
> here as well?


Also, assuming that we will use a subclass of CodeSerializer to serialize wasm,
can we check that we only get here if this is called for the subclass?

[Mircea Trofin, 2016-08-05 15:01:29]

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.cc:98: SerializeGeneric(code_object, how_to_code,
where_to_point);
On 2016/08/05 12:04:17, Yang wrote:
> Would it make sense to check for
code_object->has_reloc_info_for_serialization()
> here as well?

We don't set that flag. Should we? It seems to be user-set, rather than
calculated - not quite sure what value it brings.

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.h (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.h:50: enum SanityCheckResult {
On 2016/08/05 12:04:17, Yang wrote:
> Can we keep this in SerializedCodeData? We can make it public.

Acknowledged.

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1530:
cs.reference_map()->AddAttachedReference(*isolate->native_context());
On 2016/08/05 12:04:17, Yang wrote:
> Can we put all this into src/snapshot in a new subclass of CodeSerializer?
Aside
> from CreateCompiledModuleObject, none of this contains anything wasm-specific.

Knowing we need to attach the native context is specific (but probably not
uniquely) to wasm. 

That's actually the only extra thing that happens here. The rest is usage of a
general-purpose API.

[Yang, 2016-08-05 15:39:29]

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.cc:98: SerializeGeneric(code_object, how_to_code,
where_to_point);
On 2016/08/05 15:01:29, Mircea Trofin wrote:
> On 2016/08/05 12:04:17, Yang wrote:
> > Would it make sense to check for
> code_object->has_reloc_info_for_serialization()
> > here as well?
> 
> We don't set that flag. Should we? It seems to be user-set, rather than
> calculated - not quite sure what value it brings.

We don't emit reloc info for external references in general, unless we
explicitly make the macro assembler do it. If you emit external references in
code, but do not emit reloc info for it, serializing/deserializing between
different V8 processes is going to break.
If we emitted reloc info for external references, we set the flag so that we can
check it here.

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:28: #include "src/snapshot/deserializer.h"
Should this be removed?

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1530:
cs.reference_map()->AddAttachedReference(*isolate->native_context());
On 2016/08/05 15:01:29, Mircea Trofin wrote:
> On 2016/08/05 12:04:17, Yang wrote:
> > Can we put all this into src/snapshot in a new subclass of CodeSerializer?
> Aside
> > from CreateCompiledModuleObject, none of this contains anything
wasm-specific.
> 
> Knowing we need to attach the native context is specific (but probably not
> uniquely) to wasm. 
> 
> That's actually the only extra thing that happens here. The rest is usage of a
> general-purpose API.

Same thing can be said for CodeSerializer::Serialize. Yet the code for it is not
in src/compiler.cc. Putting it into a separate file in src/snapshot would mirror
what we do for CodeSerializer. Attaching the source string is specific to the
CodeSerializer, but belongs to the API of serializing/deserializing an
user-defined script.

I also don't like that we are using CodeSerializer to deal with wasm aside from
its original purpose. We make clear distinctions between StartupSerializer,
PartialSerializer, and CodeSerializer, each with its own purpose. Serialization
logic should not be put outside of src/snapshot.

[Mircea Trofin, 2016-08-05 20:40:18]

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

[commit-bot: I haz the power, 2016-08-05 20:40: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-05 20:43:11]

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

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

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

[Mircea Trofin, 2016-08-05 20:53:44]

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

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

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-05 21:10:59]

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

[commit-bot: I haz the power, 2016-08-05 21:11:00]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/11970)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Mircea Trofin, 2016-08-05 22:38:10]

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

[commit-bot: I haz the power, 2016-08-05 22:41:57]

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-05 23:04:36]

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

[commit-bot: I haz the power, 2016-08-05 23:04:37]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10451)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Mircea Trofin, 2016-08-06 00:31:39]

Took care of most of the feedback. has_reloc_code_for_serialization 
troubles me a bit - please take a look at my inline comments, proposed 
code change (the serialization time DCHECK), and resulting failures. Are
we seeing a bug in the cctest, or is my proposed check overly 
aggressive?

Thanks!

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.cc:98: SerializeGeneric(code_object, how_to_code,
where_to_point);
On 2016/08/05 15:39:28, Yang wrote:
> On 2016/08/05 15:01:29, Mircea Trofin wrote:
> > On 2016/08/05 12:04:17, Yang wrote:
> > > Would it make sense to check for
> > code_object->has_reloc_info_for_serialization()
> > > here as well?
> > 
> > We don't set that flag. Should we? It seems to be user-set, rather than
> > calculated - not quite sure what value it brings.
> 
> We don't emit reloc info for external references in general, unless we
> explicitly make the macro assembler do it. If you emit external references in
> code, but do not emit reloc info for it, serializing/deserializing between
> different V8 processes is going to break.
> If we emitted reloc info for external references, we set the flag so that we
can
> check it here.

I see - thanks. 

It turns out that has_reloc_code_for_serialization only applies to code->kind()
== Code::FUNCTION. Not sure if that is stale and needs to be extended, but, on
the wasm side, it would be actually expected not to have
RelocInfo::EXTERNAL_REFERENCEs. 

I added DCHECKs in the ObjectSerializer members that deal with external
references, to formalize the relationship you described.

I don't have sufficient information if we need to expand
has_reloc_code_for_serialization()'s accepting of non-FUNCTION Code objects. If
we are ok with expanding that, I'd be happy to also add a DCHECK for wasm that
ensures the flag isn't set.

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
File src/snapshot/code-serializer.h (right):

https://codereview.chromium.org/2205973003/diff/40001/src/snapshot/code-seria...
src/snapshot/code-serializer.h:50: enum SanityCheckResult {
On 2016/08/05 12:04:17, Yang wrote:
> Can we keep this in SerializedCodeData? We can make it public.

Done.

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:28: #include "src/snapshot/deserializer.h"
On 2016/08/05 15:39:28, Yang wrote:
> Should this be removed?

Done.

https://codereview.chromium.org/2205973003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1530:
cs.reference_map()->AddAttachedReference(*isolate->native_context());
On 2016/08/05 15:39:28, Yang wrote:
> On 2016/08/05 15:01:29, Mircea Trofin wrote:
> > On 2016/08/05 12:04:17, Yang wrote:
> > > Can we put all this into src/snapshot in a new subclass of CodeSerializer?
> > Aside
> > > from CreateCompiledModuleObject, none of this contains anything
> wasm-specific.
> > 
> > Knowing we need to attach the native context is specific (but probably not
> > uniquely) to wasm. 
> > 
> > That's actually the only extra thing that happens here. The rest is usage of
a
> > general-purpose API.
> 
> Same thing can be said for CodeSerializer::Serialize. Yet the code for it is
not
> in src/compiler.cc. Putting it into a separate file in src/snapshot would
mirror
> what we do for CodeSerializer. Attaching the source string is specific to the
> CodeSerializer, but belongs to the API of serializing/deserializing an
> user-defined script.
> 
> I also don't like that we are using CodeSerializer to deal with wasm aside
from
> its original purpose. We make clear distinctions between StartupSerializer,
> PartialSerializer, and CodeSerializer, each with its own purpose.
Serialization
> logic should not be put outside of src/snapshot.

Done.

I was a bit concerned that the serializer would end up learning about a very
specific (wasm) area. But I buy the consistency argument - as you pointed out,
at the moment, everything serialization-related lives under snapshot.

[Mircea Trofin, 2016-08-08 14:21:48]

Gentle reminder. 

Also, please see question in previous message.

Thanks!

[Mircea Trofin, 2016-08-08 19:30:43]

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

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

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-08 19:40:13]

To make progress, disabled the checks causing failures and 
opened v8:5274.

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

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

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

Dry run: This issue passed the CQ dry run.

[Yang, 2016-08-09 13:01:52]

On 2016/08/08 19:40:13, Mircea Trofin wrote:
> To make progress, disabled the checks causing failures and 
> opened v8:5274.

LGTM. If we can make sure that wasm code never contains external references, we
are fine. Just to be sure: by not containing external references I mean that we
do not emit raw addresses to C++ for wasm. Not having
RelocInfo::EXTERNAL_REFERENCE in the reloc info is quite common, since we only
need that reloc info for serialization. For code we do not expect to serialize,
we simply omit that reloc info.

[Mircea Trofin, 2016-08-09 14:02:21]

The CQ bit was checked by mtrofin@chromium.org

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

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-09 14:04:12]

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

[commit-bot: I haz the power, 2016-08-09 14:04:13]

Failed to commit the patch.

[Mircea Trofin, 2016-08-09 14:18:35]

The CQ bit was checked by mtrofin@chromium.org

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

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

[Mircea Trofin, 2016-08-09 14:21:44]

On 2016/08/09 13:01:52, Yang wrote:
> On 2016/08/08 19:40:13, Mircea Trofin wrote:
> > To make progress, disabled the checks causing failures and 
> > opened v8:5274.
> 
> LGTM. If we can make sure that wasm code never contains external references,
we
> are fine. Just to be sure: by not containing external references I mean that
we
> do not emit raw addresses to C++ for wasm. Not having
> RelocInfo::EXTERNAL_REFERENCE in the reloc info is quite common, since we only
> need that reloc info for serialization. For code we do not expect to
serialize,
> we simply omit that reloc info.

Makes sense. On that, though - is what we're seeing in patch #6 a bug? It looks
like
code having RelocInfo::EXTERNAL_REFERENCE doesn't also have
has_reloc_code_for_serialization.
(granted, in cctests - so maybe just a test bug).

Is that correct? If so, I'd be happy to bring back (separately) that part of the
change.

Thoughts?
Thanks!

[commit-bot: I haz the power, 2016-08-09 14:31:36]

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

[commit-bot: I haz the power, 2016-08-09 14:31:40]

Failed to commit the patch.

[Michael Achenbach, 2016-08-09 15:17:47]

The CQ bit was checked by machenbach@chromium.org

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

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-09 15:19:32]

Description was changed from

==========
[wasm] Serialization/Deserialization of compiled module

Implementation of serialization/deserialization for compiled wasm 
module. 

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

to

==========
[wasm] Serialization/Deserialization of compiled module

Implementation of serialization/deserialization for compiled wasm 
module. 

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

[commit-bot: I haz the power, 2016-08-09 15:19:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-09 15:19:50]

Description was changed from

==========
[wasm] Serialization/Deserialization of compiled module

Implementation of serialization/deserialization for compiled wasm 
module. 

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

to

==========
[wasm] Serialization/Deserialization of compiled module

Implementation of serialization/deserialization for compiled wasm
module.

BUG=v8:5072

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

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

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