lgtm It would be nice at some point to understand why we don't hit the size precisely, at least for the data and map pages.
http://codereview.chromium.org/7247002/diff/1/src/assembler.cc File src/assembler.cc (right): http://codereview.chromium.org/7247002/diff/1/src/assembler.cc#newcode101 src/assembler.cc:101: Inadvertent edit? http://codereview.chromium.org/7247002/diff/1/src/assembler.h File src/assembler.h (right): http://codereview.chromium.org/7247002/diff/1/src/assembler.h#newcode95 src/assembler.h:95: ASSERT(!is_near_linked()); Instead of this change you might find fewer conflicts later by merging r8316 from bleeding edge in its entirety. http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.cc File src/ia32/macro-assembler-ia32.cc (right): http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.cc#newcode2419 src/ia32/macro-assembler-ia32.cc:2419: test_b(Operand(instance_type), kExternalStringTag); You are making use of the fact that kExternalStringTag has only one bit set and no other string tags have that tag set, so you should assert it. I would also consider not handling external strings here. I don't think they are common enough that it is worth the complexity. http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.cc#newcode2423 src/ia32/macro-assembler-ia32.cc:2423: bind(¬_external); I think a blank line before bind(...) lines improves clarity. http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.cc#newcode2432 src/ia32/macro-assembler-ia32.cc:2432: ASSERT_EQ(SeqAsciiString::kMaxSize, SeqAsciiString::kMaxSize); I have a hard time seeing the circumstances where this assert might fail. http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.cc#newcode2448 src/ia32/macro-assembler-ia32.cc:2448: add(Operand(bitmap_scratch, MemoryChunk::kLiveBytesOffset), I wonder how the performance of this read-modify-write instruction compares with doing an add followed by a store? http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.h File src/ia32/macro-assembler-ia32.h (right): http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.h#newcode799 src/ia32/macro-assembler-ia32.h:799: // unchanged, unless it's also mask_reg. The implementation asserts that none of the registers are aliased so it seems inconsistent for this comment to describe what happens if they are. http://codereview.chromium.org/7247002/diff/1/src/ia32/stub-cache-ia32.cc File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/7247002/diff/1/src/ia32/stub-cache-ia32.cc#newcode1549 src/ia32/stub-cache-ia32.cc:1549: // TODO(gc): This only happen in new-space, where we don't I would prefer just a Note: rather than a TODO(gc): since there is no reason to expect that this will ever get fixed. http://codereview.chromium.org/7247002/diff/1/src/mark-compact.cc File src/mark-compact.cc (right): http://codereview.chromium.org/7247002/diff/1/src/mark-compact.cc#newcode317 src/mark-compact.cc:317: if (old_start == new_start) return false; A comment to the effect that the return value doesn't matter here because the adjustment to the live data count will be zero. http://codereview.chromium.org/7247002/diff/1/src/mark-compact.h File src/mark-compact.h (right): http://codereview.chromium.org/7247002/diff/1/src/mark-compact.h#newcode122 src/mark-compact.h:122: // Returns true if the new_start is marked black. Returns true if the object whose mark is transferred is marked black. http://codereview.chromium.org/7247002/diff/1/src/mark-compact.h#newcode155 src/mark-compact.h:155: // Returns true if the object whose color was transferred was black. http://codereview.chromium.org/7247002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
