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