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. http://codereview.chromium.org/548149 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
