Make InToSpace/InFromSpace use the page header.

Make InToSpace/InFromSpace use the page header.

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

[Erik Corry, 2011-05-23 12:58:56]

http://codereview.chromium.org/7058009/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7058009/diff/1/src/spaces.h#newcode1472
src/spaces.h:1472: void CopyFlagsFrom(NewSpacePage* chunk) {
This is called CopyFlagsFrom, but it only copies one flag.  I think the name
should be singular rather than plural and should include the flag that is
copied.

http://codereview.chromium.org/7058009/diff/1/src/spaces.h#newcode1560
src/spaces.h:1560: if (o->IsSmi()) {
this is SemiSpace::Contains(), but I think what we actually need (perhaps
outside of ASSERTs) is an Object::InFromSpace() and an Object::InToSpace(). 
This can be implemented with one memory read (from the page) rather than two
(one to get the id of the semispace and one to get the bit from the page).  It's
pretty performance critical in scavenging, so I think it's worth the bother of
flipping the bits after a scavenge.

[Lasse Reichstein, 2011-05-24 10:53:41]

Please re-review.

http://codereview.chromium.org/7058009/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7058009/diff/1/src/spaces.h#newcode1560
src/spaces.h:1560: if (o->IsSmi()) {
Will do. 
That also means that the CopyFlagsFrom function will be changed.

[Lasse Reichstein, 2011-05-24 10:54:57]

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc#newcode949
src/spaces.cc:949: (1 << MemoryChunk::SCAN_ON_SCAVENGE);
Vyacheslav, is this the correct flags to move? Or more or less?

[Vyacheslav Egorov (Chromium), 2011-05-24 10:58:47]

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc#newcode949
src/spaces.cc:949: (1 << MemoryChunk::SCAN_ON_SCAVENGE);
On 2011/05/24 10:54:57, Lasse Reichstein wrote:
> Vyacheslav, is this the correct flags to move? Or more or less?

I think POINTERS_FROM_HERE_ARE_INTERESTING  POINTERS_TO_HERE_ARE_INTERESTING,
SCAN_ON_SCAVENGE should be enough. Other flags should not be set on a new space
page.

I recommend moving this mask to the spaces.h where flags enum is defined so
we'll keep it up to date when we add more flags.

[Erik Corry, 2011-05-24 11:25:49]

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc#newcode1154
src/spaces.cc:1154: is_to_space_ = !is_to_space_;
This can be written in terms of GetFlag and SetFlag for more clarity.

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h#newcode379
src/spaces.h:379: IN_TO_SPACE,  // Only used if IN_NEW_SPACE is set.
Instead of a TO_SPACE flag and a FROM_SPACE flag I would suggest a TO_SPACE and
a FROM_SPACE flag.

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h#newcode1627
src/spaces.h:1627: bool is_to_space_;
This should be a SemiSpaceId instead of a bool

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h#newcode1853
src/spaces.h:1853: MemoryChunk* page = MemoryChunk::FromAddress(address);
assert it is a new space page and the same in FromSpaceContains.

[Lasse Reichstein, 2011-05-24 12:29:03]

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc#newcode949
src/spaces.cc:949: (1 << MemoryChunk::SCAN_ON_SCAVENGE);
Reduced and added as constant in NewSpacePage.

http://codereview.chromium.org/7058009/diff/3001/src/spaces.cc#newcode1154
src/spaces.cc:1154: is_to_space_ = !is_to_space_;
No, it works on the "flags" argument, not the "flags_" field.

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h#newcode379
src/spaces.h:379: IN_TO_SPACE,  // Only used if IN_NEW_SPACE is set.
Done.

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h#newcode1627
src/spaces.h:1627: bool is_to_space_;
Done.

http://codereview.chromium.org/7058009/diff/3001/src/spaces.h#newcode1853
src/spaces.h:1853: MemoryChunk* page = MemoryChunk::FromAddress(address);
Are we sure it is a new-space address?
This also works, returning false, for old-space addresses.

[Erik Corry, 2011-05-24 12:38:36]

LGTM

http://codereview.chromium.org/7058009/diff/4003/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7058009/diff/4003/src/spaces.cc#newcode1149
src/spaces.cc:1149: id_ = becomes_to_space ? kToSpace : kFromSpace;
This can all be rewritten in terms of SetFlag and ClearFlag on each page.