Another step on the way to context snapshots. We can now refer to...

Another step on the way to context snapshots.  We can now refer to
objects in the startup heap from a partial snapshot.  This happens
through the partial snapshot cache.  A startup snapshot and a
partial snapshot are created together so that the startup snapshot
contains the partial snapshot cache entries needed.

Committed: http://code.google.com/p/v8/source/detail?r=3713

[Kasper Lund, 2010-01-26 13:02:11]

LGTM with comments:

http://codereview.chromium.org/548149/diff/1/3
File src/heap.cc (right):

http://codereview.chromium.org/548149/diff/1/3#newcode3409
src/heap.cc:3409: SerDes::Iterate(v);
I'm not sure I really like the abbreviation.

http://codereview.chromium.org/548149/diff/1/3#newcode3410
src/heap.cc:3410: // We don't output a sync flag for the partial snapshot cache
in SerDes
At this point in the code the concept of 'sync flag' is kinda hard to
understand. Maybe there are just too many details here.

http://codereview.chromium.org/548149/diff/1/9
File src/heap.h (right):

http://codereview.chromium.org/548149/diff/1/9#newcode693
src/heap.h:693: // Iterates over all the other roots in the heap.
the other -> weak?

http://codereview.chromium.org/548149/diff/1/6
File src/mksnapshot.cc (right):

http://codereview.chromium.org/548149/diff/1/6#newcode167
src/mksnapshot.cc:167: i::StartupSerializer ser(&sink);
StartupSerializer -> BootstrapSerializer to better match the Bootstrapper
naming? Is there a fundamental difference between startup and bootstrapping
(bootstrapping is done for all created contexts and I assume it's the same with
your startup concept).

http://codereview.chromium.org/548149/diff/1/6#newcode170
src/mksnapshot.cc:170: i::Heap::CollectAllGarbage(true);
Only one GC here? In the test case you use two...

http://codereview.chromium.org/548149/diff/1/6#newcode171
src/mksnapshot.cc:171: ser.SerializeStrongReferences();
Refactor this pattern? Also used in other places.

http://codereview.chromium.org/548149/diff/1/8
File src/serialize.cc (right):

http://codereview.chromium.org/548149/diff/1/8#newcode845
src/serialize.cc:845: ASSERT(false);
UNREACHABLE?

http://codereview.chromium.org/548149/diff/1/8#newcode924
src/serialize.cc:924: // Don't zap the mapper yet.
There's no explicit mapper zap anymore. Does this comment still make sense?

http://codereview.chromium.org/548149/diff/1/8#newcode999
src/serialize.cc:999: int fullness = partial_snapshot_cache_fullness_;
I would post-increment partial_snapshot_cache_fullness_ here and use fullness in
the rest of the code. The mixture isn't very nice.

http://codereview.chromium.org/548149/diff/1/8#newcode1002
src/serialize.cc:1002: printf("%d: \"%s\"\n", fullness, *cstr);
This looks like debug printing. I guess this is only dumped as part of running
some tests and of course mksnapshot. Should it be hidden behind a trace flag
anyway?

http://codereview.chromium.org/548149/diff/1/8#newcode1010
src/serialize.cc:1010: return partial_snapshot_cache_fullness_++;
Isn't this really a bug? It seems nasty to update
partial_snapshot_cache_fullness_ after the recursive visit...

http://codereview.chromium.org/548149/diff/1/8#newcode1027
src/serialize.cc:1027: int offset = CurrentAllocationAddress(space) - address;
It might be nice with a comment here that explains how the offset gets encoded.
There is a lot of logic in here...

http://codereview.chromium.org/548149/diff/1/8#newcode1123
src/serialize.cc:1123: sink_->PutInt(cache_index, "root_index");
I'm not sure I understand the "root_index" string here.

http://codereview.chromium.org/548149/diff/1/8#newcode1169
src/serialize.cc:1169: serializer_->address_mapper()->Map(object_, offset);
This line illustrates that the Map() name isn't great (it's not clear it has a
side effect on the mapper). I would change it to AddMapping or something.

http://codereview.chromium.org/548149/diff/1/5
File src/serialize.h (right):

http://codereview.chromium.org/548149/diff/1/5#newcode236
src/serialize.h:236: static int partial_snapshot_cache_fullness_;
fullness is a weird name? Is it in percent? Is it a pseudo-bool? I suggest
renaming kPartialSnapshotCacheLength to capacity and change fullness to length.

http://codereview.chromium.org/548149/diff/1/5#newcode318
src/serialize.h:318: serialization_map_ = NULL;
Why do you set this to NULL? If this object is being destroyed it probably makes
little difference and it seems kinda weird.

http://codereview.chromium.org/548149/diff/1/5#newcode320
src/serialize.h:320: bool IsMapped(HeapObject* obj) {
Newline here.

http://codereview.chromium.org/548149/diff/1/5#newcode343
src/serialize.h:343: return
static_cast<int32_t>(reinterpret_cast<intptr_t>(obj->address()));
This mapper seems to be unable to handle GC/allocation because of the way it
uses addresses. Would it make sense to embed a no-allocation scope in this
class?

http://codereview.chromium.org/548149/diff/1/5#newcode355
src/serialize.h:355: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/548149/diff/1/5#newcode455
src/serialize.h:455: int fullness_[LAST_SPACE + 1];
Yeah, yeah. I know. I don't like this either.

http://codereview.chromium.org/548149/diff/1/2
File test/cctest/test-serialize.cc (right):

http://codereview.chromium.org/548149/diff/1/2#newcode352
test/cctest/test-serialize.cc:352: Heap::CollectAllGarbage(true);
Add comment that explains why you need two GCs.

[Erik Corry, 2010-01-27 08:09:35]

Now committed.

http://codereview.chromium.org/548149/diff/1/3
File src/heap.cc (right):

http://codereview.chromium.org/548149/diff/1/3#newcode3409
src/heap.cc:3409: SerDes::Iterate(v);
On 2010/01/26 13:02:11, Kasper Lund wrote:
> I'm not sure I really like the abbreviation.

Renamed to SerializerDeserializer (read it with a slash in the middle).

http://codereview.chromium.org/548149/diff/1/3#newcode3410
src/heap.cc:3410: // We don't output a sync flag for the partial snapshot cache
in SerDes
On 2010/01/26 13:02:11, Kasper Lund wrote:
> At this point in the code the concept of 'sync flag' is kinda hard to
> understand. Maybe there are just too many details here.

Comment clarified (I hope).  The reason why there is no call to v->Synchronize
here is a little subtle so I feel some detail is justified.

http://codereview.chromium.org/548149/diff/1/6
File src/mksnapshot.cc (right):

http://codereview.chromium.org/548149/diff/1/6#newcode167
src/mksnapshot.cc:167: i::StartupSerializer ser(&sink);
On 2010/01/26 13:02:11, Kasper Lund wrote:
> StartupSerializer -> BootstrapSerializer to better match the Bootstrapper
> naming? Is there a fundamental difference between startup and bootstrapping
> (bootstrapping is done for all created contexts and I assume it's the same
with
> your startup concept).

I think in that case the StartupSerializer name is correct.

http://codereview.chromium.org/548149/diff/1/6#newcode170
src/mksnapshot.cc:170: i::Heap::CollectAllGarbage(true);
There is some tuning to be done here, but not in this changelist.

http://codereview.chromium.org/548149/diff/1/6#newcode171
src/mksnapshot.cc:171: ser.SerializeStrongReferences();
On 2010/01/26 13:02:11, Kasper Lund wrote:
> Refactor this pattern? Also used in other places.

Done (and some extra cleaup).

http://codereview.chromium.org/548149/diff/1/8
File src/serialize.cc (right):

http://codereview.chromium.org/548149/diff/1/8#newcode845
src/serialize.cc:845: ASSERT(false);
On 2010/01/26 13:02:11, Kasper Lund wrote:
> UNREACHABLE?

Done

http://codereview.chromium.org/548149/diff/1/8#newcode924
src/serialize.cc:924: // Don't zap the mapper yet.
On 2010/01/26 13:02:11, Kasper Lund wrote:
> There's no explicit mapper zap anymore. Does this comment still make sense?

Comment removed.

http://codereview.chromium.org/548149/diff/1/8#newcode1002
src/serialize.cc:1002: printf("%d: \"%s\"\n", fullness, *cstr);
On 2010/01/26 13:02:11, Kasper Lund wrote:
> This looks like debug printing. I guess this is only dumped as part of running
> some tests and of course mksnapshot. Should it be hidden behind a trace flag
> anyway?

Removed.

http://codereview.chromium.org/548149/diff/1/8#newcode1010
src/serialize.cc:1010: return partial_snapshot_cache_fullness_++;
On 2010/01/26 13:02:11, Kasper Lund wrote:
> Isn't this really a bug? It seems nasty to update
> partial_snapshot_cache_fullness_ after the recursive visit...

Hopefully this isn't a bug since only the partial serializer can call this
method and the recursion takes place in the startup serializer.  But I will add
an assert to ensure that is true.

http://codereview.chromium.org/548149/diff/1/8#newcode1027
src/serialize.cc:1027: int offset = CurrentAllocationAddress(space) - address;
On 2010/01/26 13:02:11, Kasper Lund wrote:
> It might be nice with a comment here that explains how the offset gets
encoded.
> There is a lot of logic in here...

Comments added.

http://codereview.chromium.org/548149/diff/1/8#newcode1123
src/serialize.cc:1123: sink_->PutInt(cache_index, "root_index");
On 2010/01/26 13:02:11, Kasper Lund wrote:
> I'm not sure I understand the "root_index" string here.

No, it's wrong!

http://codereview.chromium.org/548149/diff/1/5
File src/serialize.h (right):

http://codereview.chromium.org/548149/diff/1/5#newcode236
src/serialize.h:236: static int partial_snapshot_cache_fullness_;
On 2010/01/26 13:02:11, Kasper Lund wrote:
> fullness is a weird name? Is it in percent? Is it a pseudo-bool? I suggest
> renaming kPartialSnapshotCacheLength to capacity and change fullness to
length.

Done

http://codereview.chromium.org/548149/diff/1/5#newcode318
src/serialize.h:318: serialization_map_ = NULL;
On 2010/01/26 13:02:11, Kasper Lund wrote:
> Why do you set this to NULL? If this object is being destroyed it probably
makes
> little difference and it seems kinda weird.

Removed.

http://codereview.chromium.org/548149/diff/1/5#newcode320
src/serialize.h:320: bool IsMapped(HeapObject* obj) {
On 2010/01/26 13:02:11, Kasper Lund wrote:
> Newline here.

Done

http://codereview.chromium.org/548149/diff/1/5#newcode343
src/serialize.h:343: return
static_cast<int32_t>(reinterpret_cast<intptr_t>(obj->address()));
On 2010/01/26 13:02:11, Kasper Lund wrote:
> This mapper seems to be unable to handle GC/allocation because of the way it
> uses addresses. Would it make sense to embed a no-allocation scope in this
> class?

Done.

http://codereview.chromium.org/548149/diff/1/5#newcode355
src/serialize.h:355: };
On 2010/01/26 13:02:11, Kasper Lund wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/548149/diff/1/5#newcode455
src/serialize.h:455: int fullness_[LAST_SPACE + 1];
On 2010/01/26 13:02:11, Kasper Lund wrote:
> Yeah, yeah. I know. I don't like this either.

Not changing now.

http://codereview.chromium.org/548149/diff/1/2
File test/cctest/test-serialize.cc (right):

http://codereview.chromium.org/548149/diff/1/2#newcode352
test/cctest/test-serialize.cc:352: Heap::CollectAllGarbage(true);
On 2010/01/26 13:02:11, Kasper Lund wrote:
> Add comment that explains why you need two GCs.

In another change.