Encode reservation meta data in the snapshot blob.

Encode reservation meta data in the snapshot blob.

This also makes reserving multiple chunks per object space possible.

R=vogelheim@chromium.org

[Yang, 2014-12-03 14:55:27]

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

[Yang, 2014-12-03 14:55:27]

Please take a look.

I wasn't able to test the external snapshot, since I have no idea how to :/

[vogelheim, 2014-12-03 18:04:09]

On 2014/12/03 14:55:27, Yang wrote:
> Please take a look.
> 
> I wasn't able to test the external snapshot, since I have no idea how to :/

Apologies for not replying to your mail on this.

Add v8_use_external_startup_data=1 to GYP_DEFINES, then recompile & run the test
suite.

[vogelheim, 2014-12-03 18:49:25]

lgtm

Looks generally good, but I'm a little spooked by the memory management. Lgtm
since I didn't see anything broken; but please consider handling allocation +
deallocation in the same class.

https://codereview.chromium.org/781443002/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/781443002/diff/1/src/serialize.cc#newcode2313
src/serialize.cc:2313: data_ = NewArray<byte>(size_);
Related to the comment in serialize.h:

I was trying to understand how the memory management works, and the only place I
found where owns_data_ is set to true was in the superclass constructor. So if I
get this right, this piece of code relies on:
- the super-class constructor creating an 'owned' NULL pointer
- then this allocates memory
- then the destructor of the super class de-allocates it.

I guess that works, but I for one had a hard time understanding it.

[Likewise for SerializedCodeData below.]

https://codereview.chromium.org/781443002/diff/1/src/serialize.cc#newcode2408
src/serialize.cc:2408: return result;
Is this expected to the last method call on a particular SerializedCodeData
instance? In that case, maybe also null-ify the data pointer when releasing
ownership.

https://codereview.chromium.org/781443002/diff/1/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/781443002/diff/1/src/serialize.h#newcode411
src/serialize.h:411: SerializedData() : data_(NULL), size_(0), owns_data_(true)
{}
Here, owns_data_ == (data_ == NULL), which doesn't seem to make sense.

If I got this right, several subclass constructors will put 'owned' data_ there
by allocating memory. In that case it may make more sense to also let those
subclasses delete that memory in their destructors.

Your choice, though...

[Yang, 2014-12-04 08:51:43]

Addressed comments.

Compiling with external snapshot uncovered some compilation issues. Test suite
fails though, with and without this patch. I assume it's because the test
harness does not pass the correct blob files to d8?

https://codereview.chromium.org/781443002/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/781443002/diff/1/src/serialize.cc#newcode2313
src/serialize.cc:2313: data_ = NewArray<byte>(size_);
On 2014/12/03 18:49:25, vogelheim wrote:
> Related to the comment in serialize.h:
> 
> I was trying to understand how the memory management works, and the only place
I
> found where owns_data_ is set to true was in the superclass constructor. So if
I
> get this right, this piece of code relies on:
> - the super-class constructor creating an 'owned' NULL pointer
> - then this allocates memory
> - then the destructor of the super class de-allocates it.
> 
> I guess that works, but I for one had a hard time understanding it.
> 
> [Likewise for SerializedCodeData below.]

I moved allocation into a helper method in the parent class, which also sets
owns_data_.

https://codereview.chromium.org/781443002/diff/1/src/serialize.cc#newcode2408
src/serialize.cc:2408: return result;
On 2014/12/03 18:49:25, vogelheim wrote:
> Is this expected to the last method call on a particular SerializedCodeData
> instance? In that case, maybe also null-ify the data pointer when releasing
> ownership.

Done.

https://codereview.chromium.org/781443002/diff/1/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/781443002/diff/1/src/serialize.h#newcode411
src/serialize.h:411: SerializedData() : data_(NULL), size_(0), owns_data_(true)
{}
On 2014/12/03 18:49:25, vogelheim wrote:
> Here, owns_data_ == (data_ == NULL), which doesn't seem to make sense.
> 
> If I got this right, several subclass constructors will put 'owned' data_
there
> by allocating memory. In that case it may make more sense to also let those
> subclasses delete that memory in their destructors.
> 
> Your choice, though...
> 
>  

Acknowledged.

[Yang, 2014-12-04 08:51:47]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-12-04 08:51:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781443002/20001

[commit-bot: I haz the power, 2014-12-04 08:53:31]

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

[commit-bot: I haz the power, 2014-12-04 08:53:31]

Try jobs failed on following builders:
  v8_linux64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/1633)
  v8_linux_arm64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1533)
  v8_linux_dbg on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/1614)
  v8_win64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/742)
  v8_win_compile_dbg on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/108)
  v8_win_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/1755)

[Yang, 2014-12-04 09:24:39]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-12-04 09:24:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781443002/40001

[commit-bot: I haz the power, 2014-12-04 09:50:35]

Committed patchset #3 (id:40001)

[Michael Achenbach, 2014-12-04 10:30:38]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/780833002/ by machenbach@chromium.org.

The reason for reverting is: [sheriff] Breaks
http://build.chromium.org/p/client.v8/builders/V8-Blink%20Linux%2064/builds/1540.