Forward white to grey transition only if slot is before the progress bar of a given object.

Forward white to grey transition only if slot is before the progress bar of a given object.


BUG=

Committed: https://code.google.com/p/v8/source/detail?r=13063

[Michael Starzinger, 2012-11-21 17:11:38]

https://codereview.chromium.org/11415070/diff/1001/src/incremental-marking-inl.h
File src/incremental-marking-inl.h (right):

https://codereview.chromium.org/11415070/diff/1001/src/incremental-marking-in...
src/incremental-marking-inl.h:47: if (reinterpret_cast<Address>(slot) -
reinterpret_cast<Address>(obj)
We don't even need to record relocation slots when the slot is right of the bar,
so we can early return with false in this case.

Also, can we have the address computation trickery as a helper function in
MemoryChunk. All the assertions could also go in there. For example with the
following signature.

bool MemoryChunk::IsAheadOfProgressBar(Object** slot);

[Hannes Payer (out of office), 2012-11-21 18:03:57]

https://codereview.chromium.org/11415070/diff/1001/src/incremental-marking-inl.h
File src/incremental-marking-inl.h (right):

https://codereview.chromium.org/11415070/diff/1001/src/incremental-marking-in...
src/incremental-marking-inl.h:47: if (reinterpret_cast<Address>(slot) -
reinterpret_cast<Address>(obj)
On 2012/11/21 17:11:38, Michael Starzinger wrote:
> We don't even need to record relocation slots when the slot is right of the
bar,
> so we can early return with false in this case.
> 
> Also, can we have the address computation trickery as a helper function in
> MemoryChunk. All the assertions could also go in there. For example with the
> following signature.
> 
> bool MemoryChunk::IsAheadOfProgressBar(Object** slot);

Done.

[Michael Starzinger, 2012-11-22 21:13:22]

https://codereview.chromium.org/11415070/diff/3003/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/11415070/diff/3003/src/spaces.h#newcode506
src/spaces.h:506: bool IsAheadOfProgressBar(Object** slot) {
Apparently we have a different understanding of the semantics of "ahead of"
because for me it means exactly the opposite of what this functions is checking.
So this proves two things:

a) We need a better name. How about IsLeftOfProgressBar?
b) We need a short comment explaining this method.

https://codereview.chromium.org/11415070/diff/3003/src/spaces.h#newcode508
src/spaces.h:508: if (reinterpret_cast<Address>(slot) -
reinterpret_cast<Address>(this) <
The pointer arithmetic is off. The progress bar actually specifies the offset
into the object, not the offset into the memory chunk. So you need to take
kObjectStartOffset into account. That would be an awfully hard to find bug.

Also can we just return the result of the comparison instead of doing an if and
the returning boolean constants?

[Hannes Payer (out of office), 2012-11-23 10:12:49]

https://codereview.chromium.org/11415070/diff/3003/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/11415070/diff/3003/src/spaces.h#newcode506
src/spaces.h:506: bool IsAheadOfProgressBar(Object** slot) {
On 2012/11/22 21:13:22, Michael Starzinger wrote:
> Apparently we have a different understanding of the semantics of "ahead of"
> because for me it means exactly the opposite of what this functions is
checking.
> So this proves two things:
> 
> a) We need a better name. How about IsLeftOfProgressBar?
> b) We need a short comment explaining this method.

Done.

https://codereview.chromium.org/11415070/diff/3003/src/spaces.h#newcode508
src/spaces.h:508: if (reinterpret_cast<Address>(slot) -
reinterpret_cast<Address>(this) <
On 2012/11/22 21:13:22, Michael Starzinger wrote:
> The pointer arithmetic is off. The progress bar actually specifies the offset
> into the object, not the offset into the memory chunk. So you need to take
> kObjectStartOffset into account. That would be an awfully hard to find bug.
> 
> Also can we just return the result of the comparison instead of doing an if
and
> the returning boolean constants?

Done.

[Michael Starzinger, 2012-11-26 15:10:26]

LGTM with a nit.

https://codereview.chromium.org/11415070/diff/2002/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/11415070/diff/2002/src/spaces.h#newcode508
src/spaces.h:508: return reinterpret_cast<Address>(slot) -
(reinterpret_cast<Address>(this) +
Used the address() helper instead of "reinterpret_cast<Address>(this)".

Also if you pull out the cast of slot to Address the code will be easier to read
and the condition will fit into one line.

[Hannes Payer (out of office), 2012-11-26 16:14:35]

https://codereview.chromium.org/11415070/diff/2002/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/11415070/diff/2002/src/spaces.h#newcode508
src/spaces.h:508: return reinterpret_cast<Address>(slot) -
(reinterpret_cast<Address>(this) +
On 2012/11/26 15:10:26, Michael Starzinger wrote:
> Used the address() helper instead of "reinterpret_cast<Address>(this)".
> 
> Also if you pull out the cast of slot to Address the code will be easier to
read
> and the condition will fit into one line.

Done.