LGTM, even though I think you should try to make smaller changes (this is a lot of changes to fairly complicated stuff).
http://codereview.chromium.org/341079/diff/1/4 File src/mksnapshot.cc (right): http://codereview.chromium.org/341079/diff/1/4#newcode197 Line 197: i::Heap::CollectAllGarbage(false); Is there any reason why you're not forcing a compaction? Maybe you should add a comment on why it's necessary to do a GC (what kind of stuff you expect to get rid off, etc.). http://codereview.chromium.org/341079/diff/1/3 File src/objects.h (right): http://codereview.chromium.org/341079/diff/1/3#newcode2913 Line 2913: static const int kCodeAlignment = 32; Express via kCodeAlignmentBits? http://codereview.chromium.org/341079/diff/1/5 File src/serialize.cc (right): http://codereview.chromium.org/341079/diff/1/5#newcode1907 Line 1907: ASSERT(NUMBER_OF_DATA_TYPES <= 16); Maybe explain why this assertion is important? http://codereview.chromium.org/341079/diff/1/5#newcode1912 Line 1912: case RAW_DATA_SERIALIZATION + index: { \ Indent \ http://codereview.chromium.org/341079/diff/1/5#newcode1986 Line 1986: int backref_space = (data & 15); Abstract out constant (15) used in multiple places to get space. Maybe even add a static inline helper that returns something of type AllocationSpace? http://codereview.chromium.org/341079/diff/1/5#newcode2005 Line 2005: case REFERENCE_SERIALIZATION + index: { \ Indent \ http://codereview.chromium.org/341079/diff/1/5#newcode2105 Line 2105: sink_->Put(character, "tagcharacter\n"); The seconds parameter to sink_->Put(...) isn't used very consistently (maybe it's on purpose). Sometimes it's all lower case, sometimes it's terminated with \n. If the \n versions have a specific meaning it might make sense to have a special Put method that adds the newline (PutSection?). http://codereview.chromium.org/341079/diff/1/5#newcode2139 Line 2139: sink_->PutInt(kPointerSize, "length"); Wouldn't it be even better to have a special serialized smi tag to avoid outputting kPointerSize everywhere? Maybe you've already tried this. http://codereview.chromium.org/341079/diff/1/5#newcode2193 Line 2193: { /* NOLINT */ Why is this scope { } necessary? http://codereview.chromium.org/341079/diff/1/5#newcode2315 Line 2315: { /* NOLINT */ Why is this scope needed? http://codereview.chromium.org/341079/diff/1/2 File src/serialize.h (right): http://codereview.chromium.org/341079/diff/1/2#newcode401 Line 401: // only works for objects in the first page of a space Terminate comment with . http://codereview.chromium.org/341079/diff/1/2#newcode434 Line 434: RAW_DATA_SERIALIZATION = 0, // And 15 common raw lengths. Maybe move the // And ... comment to the next line to better match the // Free: comments. http://codereview.chromium.org/341079/diff/1/2#newcode508 Line 508: Address high_water_[LAST_SPACE + 1]; Add separate comments on high_water_ and last_object_address_ that explain how they are used and updated. http://codereview.chromium.org/341079 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
