Estimate a (close) upper bound on the size of black-marked objects on each page.

Estimate a (close) upper bound on the size of black-marked objects on each page.

Doesn't try to be precise on new-space pages (we do tricky stuff there).

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

[Erik Corry, 2011-06-24 09:46:17]

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.c...
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.c...
src/ia32/macro-assembler-ia32.cc:2423: bind(&not_external);
I think a blank line before bind(...) lines improves clarity.

http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.c...
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.c...
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...
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#new...
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.

[Lasse Reichstein, 2011-06-24 11:09:08]

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: 
fixed.

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.c...
src/ia32/macro-assembler-ia32.cc:2419: test_b(Operand(instance_type),
kExternalStringTag);
Added assert.
Are we sure it's not more used in the browser?

http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:2423: bind(&not_external);
added

http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:2432: ASSERT_EQ(SeqAsciiString::kMaxSize,
SeqAsciiString::kMaxSize);
*Cough*
Fixed.

http://codereview.chromium.org/7247002/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:2448: add(Operand(bitmap_scratch,
MemoryChunk::kLiveBytesOffset),
Possibly a few cycle slower latency (but we're most likely not going to read it
in the next few cycles anyway), should be outweighed by the smaller code.

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...
src/ia32/macro-assembler-ia32.h:799: // unchanged, unless it's also mask_reg.
Ack, forgot to change comment back when I reverted my change to the function.

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#new...
src/ia32/stub-cache-ia32.cc:1549: // TODO(gc): This only happen in new-space,
where we don't
Done.

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;
done.

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.
Done.

http://codereview.chromium.org/7247002/diff/1/src/mark-compact.h#newcode155
src/mark-compact.h:155: 
Fixed.