[serializer] small fixes for blink snapshot.

[serializer] small fixes for blink snapshot.

Changes include:
 - Adding V8_EXPORT macro for SnapshotCreator
 - Removing outdated DCHECKs.
 - Allow nullptr as external reference. This required a...
 - Refactoring of hashmaps used by the serializer.
 - Remove external references for counters. These are not used
   anywhere for isolates that are being serialized.
 - Put template infos into the partial snapshot cache.
 - Remove unnecessary presubmit check for external references.
   mksnapshot crashes if external references are missing.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
Committed: https://crrev.com/c759a3d847b9216d7fc0e54e84eab3b31c7654f8
Cr-Commit-Position: refs/heads/master@{#40949}

[vogelheim, 2016-11-09 12:19:24]

lgtm

https://codereview.chromium.org/2490783004/diff/1/src/api.cc
File src/api.cc (left):

https://codereview.chromium.org/2490783004/diff/1/src/api.cc#oldcode1253
src/api.cc:1253: DCHECK(!i_isolate->serializer_enabled());
Ah. I'm rather certain this DCHECK is a copy of the one in #1242, since the
..WithCache logic was patterned after ..WithFastHandler. I'm not quite sure why
that one is there; but I agree they can probably both be removed. :-)

[Yang, 2016-11-10 08:25:58]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-11-10 08:26:03]

The CQ bit was unchecked by yangguo@chromium.org

[Yang, 2016-11-10 08:28:32]

Description was changed from

==========
[serializer] small fixes for blink snapshot.

- export SnapshotCreator for component build.
- allow cached function template to be serialized.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=
==========

to

==========
[serializer] small fixes for blink snapshot.

Changes include:
 - Adding V8_EXPORT macro for SnapshotCreator
 - Removing outdated DCHECKs.
 - Allow nullptr as external reference. This required a...
 - Refactoring of hashmaps used by the serializer.
 - Remove external references for counters. These are not used
   anywhere for isolates that are being serialized.
 - Put template infos into the partial snapshot cache.
 - Remove unnecessary presubmit check for external references.
   mksnapshot crashes if external references are missing.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
==========

[Yang, 2016-11-10 08:29:09]

Please take another look. I'm piggybacking a bunch of other remaining fixes onto
this.

[vogelheim, 2016-11-10 09:37:10]

lgtm

Some nitpicks, though... :-)

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h
File src/address-map.h (right):

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:17: : public base::TemplateHashMapImpl<intptr_t, uint32_t,
nitpick: why intptr_t, not uintptr_t ? 

Do we have a use for the sign, or in fact for any interpretation of the values
other than as a opaque, comparable container for 1x void* ?

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:40: static intptr_t Key(Type value) { return
reinterpret_cast<intptr_t>(value); }
nitpick: So... 'typename Type' must be a pointer type. Wouldn't it make more
sense to take a Type* as the parameter, so that typename Type can be anything,
and the map will deal with Type*?

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:200: return map_.Has(obj) ? SerializerReference(map_.Get(obj))
The way Has + Get are implemented, this Lookup performs the same operation
twice. Not sure if this is performance relevant, but why not map_.Get(..) it
once and then check for nullptr?


[The Has/Get pattern occurs a lot in this CL.]

[I find the code - as you've written it - nicer to read, but the way the map is
implemented, it's unfortunately less efficient. As said, not sure if this is
relevant.]

https://codereview.chromium.org/2490783004/diff/20001/test/cctest/test-serial...
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/2490783004/diff/20001/test/cctest/test-serial...
test/cctest/test-serialize.cc:2025: static int serialized_static_field = 314;
Huh?

[Yang, 2016-11-10 10:36:20]

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h
File src/address-map.h (right):

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:17: : public base::TemplateHashMapImpl<intptr_t, uint32_t,
On 2016/11/10 09:37:10, vogelheim wrote:
> nitpick: why intptr_t, not uintptr_t ? 
> 
> Do we have a use for the sign, or in fact for any interpretation of the values
> other than as a opaque, comparable container for 1x void* ?

No reason really. Changed.

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:40: static intptr_t Key(Type value) { return
reinterpret_cast<intptr_t>(value); }
On 2016/11/10 09:37:10, vogelheim wrote:
> nitpick: So... 'typename Type' must be a pointer type. Wouldn't it make more
> sense to take a Type* as the parameter, so that typename Type can be anything,
> and the map will deal with Type*?

Yeah, but then I can't really use PointerToIndexHashMap<Address>, but have to do
PointerToIndexHashMap<byte> instead. Besides, the class name says Pointer...

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:200: return map_.Has(obj) ? SerializerReference(map_.Get(obj))
On 2016/11/10 09:37:10, vogelheim wrote:
> The way Has + Get are implemented, this Lookup performs the same operation
> twice. Not sure if this is performance relevant, but why not map_.Get(..) it
> once and then check for nullptr?
> 
> 
> [The Has/Get pattern occurs a lot in this CL.]
> 
> [I find the code - as you've written it - nicer to read, but the way the map
is
> implemented, it's unfortunately less efficient. As said, not sure if this is
> relevant.]

Unfortunately Get is allowed to return 0 as valid result. Both Has and Get are
inline functions that call TemplateHashMapImpl<Key, Value, MatchFun,
AllocationPolicy>::Lookup, which is a const function. So I was hoping the
compiler is smart enough.

https://codereview.chromium.org/2490783004/diff/20001/test/cctest/test-serial...
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/2490783004/diff/20001/test/cctest/test-serial...
test/cctest/test-serialize.cc:2025: static int serialized_static_field = 314;
On 2016/11/10 09:37:10, vogelheim wrote:
> Huh?

We are just checking that this static address is correctly used in
serializing/deserializing below, when its address is embedded in a foreign
object

[vogelheim, 2016-11-10 10:59:28]

lgtm

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h
File src/address-map.h (right):

https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
src/address-map.h:200: return map_.Has(obj) ? SerializerReference(map_.Get(obj))
On 2016/11/10 10:36:19, Yang wrote:
> On 2016/11/10 09:37:10, vogelheim wrote:
> > The way Has + Get are implemented, this Lookup performs the same operation
> > twice. Not sure if this is performance relevant, but why not map_.Get(..) it
> > once and then check for nullptr?
> > 
> > 
> > [The Has/Get pattern occurs a lot in this CL.]
> > 
> > [I find the code - as you've written it - nicer to read, but the way the map
> is
> > implemented, it's unfortunately less efficient. As said, not sure if this is
> > relevant.]
> 
> Unfortunately Get is allowed to return 0 as valid result. Both Has and Get are
> inline functions that call TemplateHashMapImpl<Key, Value, MatchFun,
> AllocationPolicy>::Lookup, which is a const function. So I was hoping the
> compiler is smart enough.

Now that you mention it: maybe const-ify Get + Has?

I suspect const-ness isn't a sufficient guarantee for the compiler to eliminate
the 2nd call. I guess that's why the base class returns the Entry*, but that's
admittedly a somewhat ugly interface.

Either way, no big deal.

[Yang, 2016-11-11 10:54:54]

On 2016/11/10 10:59:28, vogelheim wrote:
> lgtm
> 
> https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h
> File src/address-map.h (right):
> 
>
https://codereview.chromium.org/2490783004/diff/20001/src/address-map.h#newco...
> src/address-map.h:200: return map_.Has(obj) ?
SerializerReference(map_.Get(obj))
> On 2016/11/10 10:36:19, Yang wrote:
> > On 2016/11/10 09:37:10, vogelheim wrote:
> > > The way Has + Get are implemented, this Lookup performs the same operation
> > > twice. Not sure if this is performance relevant, but why not map_.Get(..)
it
> > > once and then check for nullptr?
> > > 
> > > 
> > > [The Has/Get pattern occurs a lot in this CL.]
> > > 
> > > [I find the code - as you've written it - nicer to read, but the way the
map
> > is
> > > implemented, it's unfortunately less efficient. As said, not sure if this
is
> > > relevant.]
> > 
> > Unfortunately Get is allowed to return 0 as valid result. Both Has and Get
are
> > inline functions that call TemplateHashMapImpl<Key, Value, MatchFun,
> > AllocationPolicy>::Lookup, which is a const function. So I was hoping the
> > compiler is smart enough.
> 
> Now that you mention it: maybe const-ify Get + Has?
> 
> I suspect const-ness isn't a sufficient guarantee for the compiler to
eliminate
> the 2nd call. I guess that's why the base class returns the Entry*, but that's
> admittedly a somewhat ugly interface.
> 
> Either way, no big deal.

Get now returns a Maybe. Please take another look.

[vogelheim, 2016-11-11 16:08:15]

lgtm

Looks good, thanks!

[jochen (gone - plz use gerrit), 2016-11-12 20:58:49]

lgtm

[Yang, 2016-11-12 21:48:23]

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

[commit-bot: I haz the power, 2016-11-12 21:48:35]

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-11-12 21:50:03]

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

[commit-bot: I haz the power, 2016-11-12 21:50:04]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16104)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16147)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28418)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17656)

[Yang, 2016-11-14 06:42:13]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-11-14 06:42:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2490783004/#ps80001
(title: "rebase and fix")

[commit-bot: I haz the power, 2016-11-14 06:42:24]

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-11-14 07:17:30]

Description was changed from

==========
[serializer] small fixes for blink snapshot.

Changes include:
 - Adding V8_EXPORT macro for SnapshotCreator
 - Removing outdated DCHECKs.
 - Allow nullptr as external reference. This required a...
 - Refactoring of hashmaps used by the serializer.
 - Remove external references for counters. These are not used
   anywhere for isolates that are being serialized.
 - Put template infos into the partial snapshot cache.
 - Remove unnecessary presubmit check for external references.
   mksnapshot crashes if external references are missing.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
==========

to

==========
[serializer] small fixes for blink snapshot.

Changes include:
 - Adding V8_EXPORT macro for SnapshotCreator
 - Removing outdated DCHECKs.
 - Allow nullptr as external reference. This required a...
 - Refactoring of hashmaps used by the serializer.
 - Remove external references for counters. These are not used
   anywhere for isolates that are being serialized.
 - Put template infos into the partial snapshot cache.
 - Remove unnecessary presubmit check for external references.
   mksnapshot crashes if external references are missing.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
==========

[commit-bot: I haz the power, 2016-11-14 07:17:31]

Committed patchset #5 (id:80001)

[agrieve, 2016-11-16 19:23:53]

On 2016/11/14 07:17:31, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001)

fyi - This increased android's v8 snapshot by 36kb. (just making sure this is
expected)

[Yang, 2016-11-16 20:17:49]

On 2016/11/16 19:23:53, agrieve wrote:
> On 2016/11/14 07:17:31, commit-bot: I haz the power wrote:
> > Committed patchset #5 (id:80001)
> 
> fyi - This increased android's v8 snapshot by 36kb. (just making sure this is
> expected)

I would not say that this is expected. It does not show up in the graph here:
https://chromeperf.appspot.com/report?sid=f0b049cef98c4559a594d0e9519e24e7a55...

If it's not too much to ask, could you compile v8's cctest compile target and
run `cctest SerializationMemoryStats` from before and after this CL to make
sure? Please file a bug. Thanks. I will take a look at this tomorrow if I get
around.

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

Description was changed from

==========
[serializer] small fixes for blink snapshot.

Changes include:
 - Adding V8_EXPORT macro for SnapshotCreator
 - Removing outdated DCHECKs.
 - Allow nullptr as external reference. This required a...
 - Refactoring of hashmaps used by the serializer.
 - Remove external references for counters. These are not used
   anywhere for isolates that are being serialized.
 - Put template infos into the partial snapshot cache.
 - Remove unnecessary presubmit check for external references.
   mksnapshot crashes if external references are missing.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
==========

to

==========
[serializer] small fixes for blink snapshot.

Changes include:
 - Adding V8_EXPORT macro for SnapshotCreator
 - Removing outdated DCHECKs.
 - Allow nullptr as external reference. This required a...
 - Refactoring of hashmaps used by the serializer.
 - Remove external references for counters. These are not used
   anywhere for isolates that are being serialized.
 - Put template infos into the partial snapshot cache.
 - Remove unnecessary presubmit check for external references.
   mksnapshot crashes if external references are missing.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892

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

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

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