Cardmarking writebarrier.

- Changes to enable oldspaces iteration without maps decoding:
-- layout change for FixedArrays: length is stored as a smis (initial patch by Kevin Millikin)
-- layout change for SharedFunctionInfo: integer fields are stored as smi on arm, ia32 and rearranged on x64.
-- layout change for String: meaning of LSB bit is fliped (1 now means hash not computed); on x64 padding is added. 
-- layout of maps is _not_ changed. Map space is currently iterated in a special way. 
- Cardmarking write barrier. New barrier handles large objects and normal objects in a similar fashion (no more additional space for pointer tracking is required, no conditional branches in WB code).

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

[Kasper Lund, 2010-05-17 10:38:24]

Quick comment:

http://codereview.chromium.org/2101002/diff/2001/3015
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3015#newcode67
src/ia32/macro-assembler-ia32.cc:67: bts(Operand(scratch), addr);
Can't you use:

   bts(Operand(object, Page::kDirtyFlagOffset), addr)?

and get rid of two moves?

[Mads Ager (chromium), 2010-05-17 14:18:39]

Large change. First batch of comments. :)

http://codereview.chromium.org/2101002/diff/2001/3003
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3003#newcode768
src/arm/ic-arm.cc:768: // r2: untagged index
Update r2 comment. No longer holds untagged index at this point.

http://codereview.chromium.org/2101002/diff/2001/3003#newcode787
src/arm/ic-arm.cc:787: // r2: untagged index
Update r2 comment. No longer holds untagged index at this point.

http://codereview.chromium.org/2101002/diff/2001/3007
File src/globals.h (right):

http://codereview.chromium.org/2101002/diff/2001/3007#newcode536
src/globals.h:536: // MAP_POINTER_ALIGN returns the value aligned as a map
pointer.
Do you want to change the name of OBJECT_SIZE_ALIGN as well? This is the same
operation for maps so the naming should be consistent.

http://codereview.chromium.org/2101002/diff/2001/3008
File src/heap-inl.h (right):

http://codereview.chromium.org/2101002/diff/2001/3008#newcode245
src/heap-inl.h:245: void Heap::CopyBlockToOldSpaceAndUpdateRegionsMarks(Address
dst,
RegionsMarks -> RegionMarks?

http://codereview.chromium.org/2101002/diff/2001/3008#newcode280
src/heap-inl.h:280: Object** src_object_p = reinterpret_cast<Object**>(src);
I don't like the _p suffix. We are not using that naming convention anywhere
else.

http://codereview.chromium.org/2101002/diff/2001/3008#newcode293
src/heap-inl.h:293: void Heap::MoveBlockToOldSpaceAndUpdateRegionsMarks(Address
dst,
RegionsMarks -> RegionMarks

Also, isn't this method identical to CopyBlockToOldSpaceAndUpdateRegionMarks
except for the second assert and the for/while difference? We should have only
one of these or share the implementation?

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode811
src/heap.cc:811: VerifyPageWatermarkValidity(old_pointer_space_, true);
Use enum with values: VALID, INVALID instead of the boolean to make these call
sites easier to read.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode1298
src/heap.cc:1298: obj = AllocateMap(BYTE_ARRAY_TYPE, ByteArray::kHeaderSize);
This change looks a little bit spooky? All the other map allocations here use
the aligned size of objects. Is this safe and if it is, could you add a comment
to explain the difference?

http://codereview.chromium.org/2101002/diff/2001/3009#newcode1578
src/heap.cc:1578: obj = AllocateSymbol(CStrVector(""), 0, 0);
Moving from a named constant to an integer seems like a step in the wrong
direction?

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3265
src/heap.cc:3265: ) {
Indentation.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3269
src/heap.cc:3269: Object** object_p =
reinterpret_cast<Object**>(object_address);
Remove _p suffix?

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3369
src/heap.cc:3369: ) {
Indentation.  Similar for the rest of the function in this file.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3374
src/heap.cc:3374: Object** object_p =
reinterpret_cast<Object**>(object_address);
Remove _p suffix.  Ditto for the rest of this file.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3389
src/heap.cc:3389: return page + (((addr - page) + (Map::kSize - 1))/Map::kSize *
Map::kSize);
Space around operators: /

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3396
src/heap.cc:3396: return page + ((addr - page)/Map::kSize * Map::kSize);
Space around binary operators: /

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3513
src/heap.cc:3513: return newmarks;
Either use a one-liner or have braces around the body of the if.

http://codereview.chromium.org/2101002/diff/2001/3010
File src/heap.h (right):

http://codereview.chromium.org/2101002/diff/2001/3010#newcode757
src/heap.h:757: bool can_preallocate_during_iteration);
Why is this called preallocate? Should it just be can_allocate_during_iteration?
Also, would it make sense to have use an enum to get a meaningful name to use at
callsites?

[Vyacheslav Egorov (Chromium), 2010-05-17 15:01:35]

http://codereview.chromium.org/2101002/diff/2001/3008
File src/heap-inl.h (right):

http://codereview.chromium.org/2101002/diff/2001/3008#newcode280
src/heap-inl.h:280: Object** src_object_p = reinterpret_cast<Object**>(src);
On 2010/05/17 14:18:39, Mads Ager wrote:
> I don't like the _p suffix. We are not using that naming convention anywhere
> else.

Well it was inherited from IterateRSetRange. I don't like suffix either but I
think that name 'object' does not reflect variable meaning. 

I propose renaming to 'slot' or something similar. What do you think?

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3265
src/heap.cc:3265: ) {
On 2010/05/17 14:18:39, Mads Ager wrote:
> Indentation.

Strange that linter does not catch this style violation. I had to check style
guide to understand how it should be indented.

http://codereview.chromium.org/2101002/diff/2001/3010
File src/heap.h (right):

http://codereview.chromium.org/2101002/diff/2001/3010#newcode757
src/heap.h:757: bool can_preallocate_during_iteration);
On 2010/05/17 14:18:39, Mads Ager wrote:
> Why is this called preallocate? Should it just be
can_allocate_during_iteration?
> Also, would it make sense to have use an enum to get a meaningful name to use
at
> callsites?

During scavenge allocation in old space and initialization of allocated space is
separated in time. That is why I decided to call it 'preallocate'. 

I will try to find another meaningful name for it.

[antonm, 2010-05-17 15:14:23]

drive-by comments

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3374
src/heap.cc:3374: Object** object_p =
reinterpret_cast<Object**>(object_address);
do not you want to assert that all the objects are now heap objects (here and in
other verify functions)?

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3389
src/heap.cc:3389: return page + (((addr - page) + (Map::kSize - 1))/Map::kSize *
Map::kSize);
Map::kSize should be a power of two, no?  Maybe use bit masking then?

http://codereview.chromium.org/2101002/diff/2001/3018
File src/mark-compact.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3018#newcode1472
src/mark-compact.cc:1472: // Page::ClearRegionMarks(start,
commented out?

http://codereview.chromium.org/2101002/diff/2001/3027
File src/spaces.h (right):

http://codereview.chromium.org/2101002/diff/2001/3027#newcode230
src/spaces.h:230: static const int kPageHeaderSize = kPointerSize + kPointerSize
+ kIntSize +
do we want to check/ensure it is properly aligned?

http://codereview.chromium.org/2101002/diff/2001/3029
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3029#newcode4553
src/x64/codegen-x64.cc:4553: __ SmiToInteger32(rbx, rbx);
now this part of code probably could be simplified---if I remember it correctly
I needed two registers precisely for sometimes it is more convenient to work
with smis and sometimes with ints.  But I am not 100% sure.

Could you file a bug and assign it to me when this CL is committed?

[Vyacheslav Egorov (Chromium), 2010-05-17 15:37:01]

Thanks for your comments, Anton!

This change is large and employs some dirty tricks so the more eyes the better!

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3389
src/heap.cc:3389: return page + (((addr - page) + (Map::kSize - 1))/Map::kSize *
Map::kSize);
On 2010/05/17 15:14:23, antonm wrote:
> Map::kSize should be a power of two, no?  Maybe use bit masking then?

It is 32 on ia32 but unfortunately it is 48 bytes on x64. 
And that is exactly the reason why I had to do some black magic here [region
size is not divisible by 48].

http://codereview.chromium.org/2101002/diff/2001/3027
File src/spaces.h (right):

http://codereview.chromium.org/2101002/diff/2001/3027#newcode230
src/spaces.h:230: static const int kPageHeaderSize = kPointerSize + kPointerSize
+ kIntSize +
On 2010/05/17 15:14:23, antonm wrote:
> do we want to check/ensure it is properly aligned?

Well kHeaderSize is not necessary aligned.
But kObjectStartOffset is aligned.

Do you propose to rename these constants?

[antonm, 2010-05-17 15:40:34]

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3389
src/heap.cc:3389: return page + (((addr - page) + (Map::kSize - 1))/Map::kSize *
Map::kSize);
On 2010/05/17 15:37:01, Vyacheslav Egorov wrote:
> On 2010/05/17 15:14:23, antonm wrote:
> > Map::kSize should be a power of two, no?  Maybe use bit masking then?
> 
> It is 32 on ia32 but unfortunately it is 48 bytes on x64. 
> And that is exactly the reason why I had to do some black magic here [region
> size is not divisible by 48].
> 

You're right, I forgot that additional padding was only for ia32.

http://codereview.chromium.org/2101002/diff/2001/3027
File src/spaces.h (right):

http://codereview.chromium.org/2101002/diff/2001/3027#newcode230
src/spaces.h:230: static const int kPageHeaderSize = kPointerSize + kPointerSize
+ kIntSize +
On 2010/05/17 15:37:01, Vyacheslav Egorov wrote:
> On 2010/05/17 15:14:23, antonm wrote:
> > do we want to check/ensure it is properly aligned?
> 
> Well kHeaderSize is not necessary aligned.
> But kObjectStartOffset is aligned.
> 
> Do you propose to rename these constants?

Nope, just wanted to double check if it's fine :)

[iposva, 2010-05-17 17:48:52]

In the future it might make sense to break changes like these into smaller
pieces. For example the different Smi related cleanups in the objects could have
been pulled out of this change into their own individual changes in preparation
of the card-marking changes. Breaking changes up into smaller pieces has the
advantage of smaller and more focused reviews.

This feedback has two sections:
- Actual code feedback in macro-assembler-ia32.cc which likely also applies to
other architectures.
- Many comments about comments in spaces.h. (I have not looked at other files
for similar issues in the comments.)

Cheers,
-Ivan

http://codereview.chromium.org/2101002/diff/2001/3015
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3015#newcode64
src/ia32/macro-assembler-ia32.cc:64: mov(scratch, Operand(object,
Page::kDirtyFlagOffset));
While the code here may seem logical to you as the author at the moment of
writing, I find that for code with this much "internal knowledge" of the VM it
helps to add a lot of explanatory comments. As a rule of thumb for this kind of
code I would expect to write about 1.5 lines of comments for each line of actual
code (aka assembly instruction).

For example: It is entirely unclear how this deals with large objects where the
destination is outside the regular page size. I can only assume that you mask
off the higher offsets and set the corresponding bit in the low address and thus
you visit all the possible regions for large pages?

http://codereview.chromium.org/2101002/diff/2001/3015#newcode130
src/ia32/macro-assembler-ia32.cc:130: if ((offset > 0) && (offset <
Page::kMaxHeapObjectSize - kHeapObjectTag)) {
This whole test and the comments above seem not needed any longer. The code
inside the "if ((offset > 0) ..." and in the first part of the else appear
identical. It would make sense to simplify this code now.

http://codereview.chromium.org/2101002/diff/2001/3027
File src/spaces.h (right):

http://codereview.chromium.org/2101002/diff/2001/3027#newcode62
src/spaces.h:62: // size. Each regions has a corresponding dirty bit in page
header which is
regions -> region

http://codereview.chromium.org/2101002/diff/2001/3027#newcode63
src/spaces.h:63: // set if region might contain pointers to new space.  During
scavenges and
if region -> if the region

http://codereview.chromium.org/2101002/diff/2001/3027#newcode65
src/spaces.h:65: // heap object maps so if page belongs to old pointer space or
large object
if page -> if the page
or 
if page -> if a page

http://codereview.chromium.org/2101002/diff/2001/3027#newcode66
src/spaces.h:66: // space it is essential to guarantee that it does not contain
any garbage
that it does: the "it" is ambiguous as it is not immediately clear from the
context that the page is meant.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode68
src/spaces.h:68: // Heap::InNewSpace() predicate must be a pointer to alive heap
object from new
Heap::InNewSpace() predicat -> the Heap::InNewSpace() predicate

alive ... -> a live heap object in new space

http://codereview.chromium.org/2101002/diff/2001/3027#newcode79
src/spaces.h:79: // into special field in page header and set a page
WATERMARK_INVALIDATED flag.
in page header -> in the page header

http://codereview.chromium.org/2101002/diff/2001/3027#newcode157
src/spaces.h:157: // Return allocation watermark for the page.
Return allocation watermark -> Return the allocation watermark

http://codereview.chromium.org/2101002/diff/2001/3027#newcode158
src/spaces.h:158: // For old space pages it is guaranteed that area under
watermark
that area under watermark -> that the area under the watermark

http://codereview.chromium.org/2101002/diff/2001/3027#newcode162
src/spaces.h:162: // Return allocation watermark offset from the beginning of
the page.
Return the allocation watermark

http://codereview.chromium.org/2101002/diff/2001/3027#newcode258
src/spaces.h:258: // To avoid additional WATERMARK_INVALIDATED flag clearing
pass during
avoid an additional

http://codereview.chromium.org/2101002/diff/2001/3027#newcode259
src/spaces.h:259: // scavenge we just invalidate watermark on each old space
page after
invalidate the watermark

http://codereview.chromium.org/2101002/diff/2001/3027#newcode260
src/spaces.h:260: // processing it. And then we flip meaning of the
WATERMARK_INVALIDATED flag
flip the meaning

http://codereview.chromium.org/2101002/diff/2001/3027#newcode261
src/spaces.h:261: // at the begging of the next scavenge and each page becomes
marked as having
begging ?-> beginning

http://codereview.chromium.org/2101002/diff/2001/3027#newcode262
src/spaces.h:262: // valid watermark.
a valid watermark

http://codereview.chromium.org/2101002/diff/2001/3027#newcode265
src/spaces.h:265: // Returns true if page allocation watermark was not altered
during scavenge.
if the page

http://codereview.chromium.org/2101002/diff/2001/3027#newcode286
src/spaces.h:286: // This field contains meaning of WATERMARK_INVALIDATED flag.
contains the meaning of the WATERMARK_INVALIDATED flag

http://codereview.chromium.org/2101002/diff/2001/3027#newcode288
src/spaces.h:288: // it's meaning at the beginning of scavenge.
of a scavenge

[Mads Ager (chromium), 2010-05-18 08:28:50]

This looks great! A few more comments for you.

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3572
src/heap.cc:3572: Address end    =
Remove extra spacing.

http://codereview.chromium.org/2101002/diff/2001/3011
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3011#newcode227
src/ia32/builtins-ia32.cc:227: __ mov(Operand(edi, HeapObject::kMapOffset),
eax);  // setup the map
[Edit: I see now that the Array super class has been removed. Why?]

This will work just fine, but it seems a bit strange to change this to the super
class HeapObject when you know that it is a JSObject and then below change from
super class Array to sub class FixedArray. Let's either use the most specific
type or use the type in which the offsets are defined?

http://codereview.chromium.org/2101002/diff/2001/3011#newcode760
src/ia32/builtins-ia32.cc:760: __ mov(FieldOperand(scratch1,
HeapObject::kMapOffset),
Ditto.

http://codereview.chromium.org/2101002/diff/2001/3011#newcode854
src/ia32/builtins-ia32.cc:854: __ mov(FieldOperand(elements_array,
HeapObject::kMapOffset),
Ditto.

http://codereview.chromium.org/2101002/diff/2001/3015
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3015#newcode96
src/ia32/macro-assembler-ia32.cc:96: // Set the remembered set bit for
[object+offset].
We no longer use remembered sets so this comment should be revised. We should
make sure to search for |remembered set| in the code base and update all
comments.

http://codereview.chromium.org/2101002/diff/2001/3018
File src/mark-compact.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3018#newcode1439
src/mark-compact.cc:1439: // Address end = start + size_in_bytes;
Code in comments.

http://codereview.chromium.org/2101002/diff/2001/3018#newcode1491
src/mark-compact.cc:1491: // Page::ClearRegionMarks(start,
Code in comments.

http://codereview.chromium.org/2101002/diff/2001/3018#newcode2056
src/mark-compact.cc:2056: // Find end of allocation of in the page of
first_forwarded.
allocation of in the -> allocation in the

http://codereview.chromium.org/2101002/diff/2001/3021
File src/objects-inl.h (right):

http://codereview.chromium.org/2101002/diff/2001/3021#newcode763
src/objects-inl.h:763: IsRegionDirty(object->address() + offset));      \
This line should be indented more.

http://codereview.chromium.org/2101002/diff/2001/3023
File src/objects.h (left):

http://codereview.chromium.org/2101002/diff/2001/3023#oldcode58
src/objects.h:58: //       - Array
Why remove the Array super class here? All the arrays need lengths and they all
need to be smi-tagged, right?

http://codereview.chromium.org/2101002/diff/2001/3026
File src/spaces.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3026#newcode2696
src/spaces.cc:2696: uint32_t marks    = page->GetRegionMarks();
Remove extra indentation here and in the rest of this method.

http://codereview.chromium.org/2101002/diff/2001/3027
File src/spaces.h (right):

http://codereview.chromium.org/2101002/diff/2001/3027#newcode242
src/spaces.h:242: static const int kDirtyFlagOffset = 2*kPointerSize;
Space around binary op.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode244
src/spaces.h:244: static const int kRegionSize      = 1 << kRegionSizeLog2;
I would remove the extra spacing here.

[Vyacheslav Egorov (Chromium), 2010-05-18 08:39:50]

http://codereview.chromium.org/2101002/diff/2001/3023
File src/objects.h (left):

http://codereview.chromium.org/2101002/diff/2001/3023#oldcode58
src/objects.h:58: //       - Array
On 2010/05/18 08:28:50, Mads Ager wrote:
> Why remove the Array super class here? All the arrays need lengths and they
all
> need to be smi-tagged, right?

Not all array length need to be smi-tagged because some of does not contain any
references so they will never appear in old pointer space.

[Vyacheslav Egorov (Chromium), 2010-05-19 11:50:57]

I addressed comments about comments by adding more comments. :)

http://codereview.chromium.org/2101002/diff/2001/3003
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3003#newcode768
src/arm/ic-arm.cc:768: // r2: untagged index
On 2010/05/17 14:18:39, Mads Ager wrote:
> Update r2 comment. No longer holds untagged index at this point.

Done.

http://codereview.chromium.org/2101002/diff/2001/3003#newcode787
src/arm/ic-arm.cc:787: // r2: untagged index
On 2010/05/17 14:18:39, Mads Ager wrote:
> Update r2 comment. No longer holds untagged index at this point.

Done.

http://codereview.chromium.org/2101002/diff/2001/3007
File src/globals.h (right):

http://codereview.chromium.org/2101002/diff/2001/3007#newcode536
src/globals.h:536: // MAP_POINTER_ALIGN returns the value aligned as a map
pointer.
On 2010/05/17 14:18:39, Mads Ager wrote:
> Do you want to change the name of OBJECT_SIZE_ALIGN as well? This is the same
> operation for maps so the naming should be consistent.

Yep. *_SIZE_ALIGN is a very misleading name. I spent some time banging my head
against the wall because of it.

http://codereview.chromium.org/2101002/diff/2001/3008
File src/heap-inl.h (right):

http://codereview.chromium.org/2101002/diff/2001/3008#newcode245
src/heap-inl.h:245: void Heap::CopyBlockToOldSpaceAndUpdateRegionsMarks(Address
dst,
On 2010/05/17 14:18:39, Mads Ager wrote:
> RegionsMarks -> RegionMarks?

Done.

http://codereview.chromium.org/2101002/diff/2001/3008#newcode280
src/heap-inl.h:280: Object** src_object_p = reinterpret_cast<Object**>(src);
On 2010/05/17 15:01:35, Vyacheslav Egorov wrote:
> On 2010/05/17 14:18:39, Mads Ager wrote:
> > I don't like the _p suffix. We are not using that naming convention anywhere
> > else.
> 
> Well it was inherited from IterateRSetRange. I don't like suffix either but I
> think that name 'object' does not reflect variable meaning. 
> 
> I propose renaming to 'slot' or something similar. What do you think? 
> 
> 

Done.

http://codereview.chromium.org/2101002/diff/2001/3008#newcode293
src/heap-inl.h:293: void Heap::MoveBlockToOldSpaceAndUpdateRegionsMarks(Address
dst,
On 2010/05/17 14:18:39, Mads Ager wrote:
> RegionsMarks -> RegionMarks
> 
> Also, isn't this method identical to CopyBlockToOldSpaceAndUpdateRegionMarks
> except for the second assert and the for/while difference? We should have only
> one of these or share the implementation?

Done.

http://codereview.chromium.org/2101002/diff/2001/3009
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3009#newcode811
src/heap.cc:811: VerifyPageWatermarkValidity(old_pointer_space_, true);
On 2010/05/17 14:18:39, Mads Ager wrote:
> Use enum with values: VALID, INVALID instead of the boolean to make these call
> sites easier to read.

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode1298
src/heap.cc:1298: obj = AllocateMap(BYTE_ARRAY_TYPE, ByteArray::kHeaderSize);
On 2010/05/17 14:18:39, Mads Ager wrote:
> This change looks a little bit spooky? All the other map allocations here use
> the aligned size of objects. Is this safe and if it is, could you add a
comment
> to explain the difference?

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode1578
src/heap.cc:1578: obj = AllocateSymbol(CStrVector(""), 0, 0);
On 2010/05/17 14:18:39, Mads Ager wrote:
> Moving from a named constant to an integer seems like a step in the wrong
> direction?

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3265
src/heap.cc:3265: ) {
On 2010/05/17 15:01:35, Vyacheslav Egorov wrote:
> On 2010/05/17 14:18:39, Mads Ager wrote:
> > Indentation.
> 
> Strange that linter does not catch this style violation. I had to check style
> guide to understand how it should be indented. 

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3269
src/heap.cc:3269: Object** object_p =
reinterpret_cast<Object**>(object_address);
On 2010/05/17 14:18:39, Mads Ager wrote:
> Remove _p suffix?

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3369
src/heap.cc:3369: ) {
On 2010/05/17 14:18:39, Mads Ager wrote:
> Indentation.  Similar for the rest of the function in this file.

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3374
src/heap.cc:3374: Object** object_p =
reinterpret_cast<Object**>(object_address);
On 2010/05/17 15:14:23, antonm wrote:
> do not you want to assert that all the objects are now heap objects (here and
in
> other verify functions)?

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3389
src/heap.cc:3389: return page + (((addr - page) + (Map::kSize - 1))/Map::kSize *
Map::kSize);
On 2010/05/17 14:18:39, Mads Ager wrote:
> Space around operators: /

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3396
src/heap.cc:3396: return page + ((addr - page)/Map::kSize * Map::kSize);
On 2010/05/17 14:18:39, Mads Ager wrote:
> Space around binary operators: /

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3513
src/heap.cc:3513: return newmarks;
On 2010/05/17 14:18:39, Mads Ager wrote:
> Either use a one-liner or have braces around the body of the if.

Done.

http://codereview.chromium.org/2101002/diff/2001/3009#newcode3572
src/heap.cc:3572: Address end    =
On 2010/05/18 08:28:50, Mads Ager wrote:
> Remove extra spacing.

Done.

http://codereview.chromium.org/2101002/diff/2001/3010
File src/heap.h (right):

http://codereview.chromium.org/2101002/diff/2001/3010#newcode757
src/heap.h:757: bool can_preallocate_during_iteration);
On 2010/05/17 15:01:35, Vyacheslav Egorov wrote:
> On 2010/05/17 14:18:39, Mads Ager wrote:
> > Why is this called preallocate? Should it just be
> can_allocate_during_iteration?
> > Also, would it make sense to have use an enum to get a meaningful name to
use
> at
> > callsites?
> 
> During scavenge allocation in old space and initialization of allocated space
is
> separated in time. That is why I decided to call it 'preallocate'. 
> 
> I will try to find another meaningful name for it. 
> 

Done.

http://codereview.chromium.org/2101002/diff/2001/3011
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3011#newcode227
src/ia32/builtins-ia32.cc:227: __ mov(Operand(edi, HeapObject::kMapOffset),
eax);  // setup the map
On 2010/05/18 08:28:50, Mads Ager wrote:
> [Edit: I see now that the Array super class has been removed. Why?]
> 
> This will work just fine, but it seems a bit strange to change this to the
super
> class HeapObject when you know that it is a JSObject and then below change
from
> super class Array to sub class FixedArray. Let's either use the most specific
> type or use the type in which the offsets are defined?

Well. It is not a JSObject it is actually FixedArray. Changed
HeapObject::kMapOffset to FixedArray::kMapOffset

http://codereview.chromium.org/2101002/diff/2001/3011#newcode760
src/ia32/builtins-ia32.cc:760: __ mov(FieldOperand(scratch1,
HeapObject::kMapOffset),
On 2010/05/18 08:28:50, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/2101002/diff/2001/3011#newcode854
src/ia32/builtins-ia32.cc:854: __ mov(FieldOperand(elements_array,
HeapObject::kMapOffset),
On 2010/05/18 08:28:50, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/2101002/diff/2001/3015
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3015#newcode64
src/ia32/macro-assembler-ia32.cc:64: mov(scratch, Operand(object,
Page::kDirtyFlagOffset));
On 2010/05/17 17:48:53, iposva wrote:
> While the code here may seem logical to you as the author at the moment of
> writing, I find that for code with this much "internal knowledge" of the VM it
> helps to add a lot of explanatory comments. As a rule of thumb for this kind
of
> code I would expect to write about 1.5 lines of comments for each line of
actual
> code (aka assembly instruction).
> 
> For example: It is entirely unclear how this deals with large objects where
the
> destination is outside the regular page size. I can only assume that you mask
> off the higher offsets and set the corresponding bit in the low address and
thus
> you visit all the possible regions for large pages?

Done.

http://codereview.chromium.org/2101002/diff/2001/3015#newcode67
src/ia32/macro-assembler-ia32.cc:67: bts(Operand(scratch), addr);
On 2010/05/17 10:38:24, Kasper Lund wrote:
> Can't you use:
> 
>    bts(Operand(object, Page::kDirtyFlagOffset), addr)?
> 
> and get rid of two moves?

I measured the difference. It is negligible and bts(mem, reg) is sometimes
faster so I will use it.

http://codereview.chromium.org/2101002/diff/2001/3015#newcode96
src/ia32/macro-assembler-ia32.cc:96: // Set the remembered set bit for
[object+offset].
On 2010/05/18 08:28:50, Mads Ager wrote:
> We no longer use remembered sets so this comment should be revised. We should
> make sure to search for |remembered set| in the code base and update all
> comments.

Comments cleanup done (except for MIPS port).

http://codereview.chromium.org/2101002/diff/2001/3015#newcode130
src/ia32/macro-assembler-ia32.cc:130: if ((offset > 0) && (offset <
Page::kMaxHeapObjectSize - kHeapObjectTag)) {
On 2010/05/17 17:48:53, iposva wrote:
> This whole test and the comments above seem not needed any longer. The code
> inside the "if ((offset > 0) ..." and in the first part of the else appear
> identical. It would make sense to simplify this code now.

Done.

http://codereview.chromium.org/2101002/diff/2001/3018
File src/mark-compact.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3018#newcode1439
src/mark-compact.cc:1439: // Address end = start + size_in_bytes;
On 2010/05/18 08:28:50, Mads Ager wrote:
> Code in comments.

Done.

http://codereview.chromium.org/2101002/diff/2001/3018#newcode1472
src/mark-compact.cc:1472: // Page::ClearRegionMarks(start,
On 2010/05/17 15:14:23, antonm wrote:
> commented out?

Done.

http://codereview.chromium.org/2101002/diff/2001/3018#newcode1491
src/mark-compact.cc:1491: // Page::ClearRegionMarks(start,
On 2010/05/18 08:28:50, Mads Ager wrote:
> Code in comments.

Done.

http://codereview.chromium.org/2101002/diff/2001/3018#newcode2056
src/mark-compact.cc:2056: // Find end of allocation of in the page of
first_forwarded.
On 2010/05/18 08:28:50, Mads Ager wrote:
> allocation of in the -> allocation in the

Done.

http://codereview.chromium.org/2101002/diff/2001/3021
File src/objects-inl.h (right):

http://codereview.chromium.org/2101002/diff/2001/3021#newcode763
src/objects-inl.h:763: IsRegionDirty(object->address() + offset));      \
On 2010/05/18 08:28:50, Mads Ager wrote:
> This line should be indented more. 

Done.

http://codereview.chromium.org/2101002/diff/2001/3026
File src/spaces.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3026#newcode2696
src/spaces.cc:2696: uint32_t marks    = page->GetRegionMarks();
On 2010/05/18 08:28:50, Mads Ager wrote:
> Remove extra indentation here and in the rest of this method.

Done.

http://codereview.chromium.org/2101002/diff/2001/3027
File src/spaces.h (right):

http://codereview.chromium.org/2101002/diff/2001/3027#newcode62
src/spaces.h:62: // size. Each regions has a corresponding dirty bit in page
header which is
On 2010/05/17 17:48:53, iposva wrote:
> regions -> region

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode63
src/spaces.h:63: // set if region might contain pointers to new space.  During
scavenges and
On 2010/05/17 17:48:53, iposva wrote:
> if region -> if the region

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode65
src/spaces.h:65: // heap object maps so if page belongs to old pointer space or
large object
On 2010/05/17 17:48:53, iposva wrote:
> if page -> if the page
> or 
> if page -> if a page

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode66
src/spaces.h:66: // space it is essential to guarantee that it does not contain
any garbage
On 2010/05/17 17:48:53, iposva wrote:
> that it does: the "it" is ambiguous as it is not immediately clear from the
> context that the page is meant.

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode68
src/spaces.h:68: // Heap::InNewSpace() predicate must be a pointer to alive heap
object from new
On 2010/05/17 17:48:53, iposva wrote:
> Heap::InNewSpace() predicat -> the Heap::InNewSpace() predicate
> 
> alive ... -> a live heap object in new space

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode79
src/spaces.h:79: // into special field in page header and set a page
WATERMARK_INVALIDATED flag.
On 2010/05/17 17:48:53, iposva wrote:
> in page header -> in the page header

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode157
src/spaces.h:157: // Return allocation watermark for the page.
On 2010/05/17 17:48:53, iposva wrote:
> Return allocation watermark -> Return the allocation watermark

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode158
src/spaces.h:158: // For old space pages it is guaranteed that area under
watermark
On 2010/05/17 17:48:53, iposva wrote:
> that area under watermark -> that the area under the watermark

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode162
src/spaces.h:162: // Return allocation watermark offset from the beginning of
the page.
On 2010/05/17 17:48:53, iposva wrote:
> Return the allocation watermark

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode242
src/spaces.h:242: static const int kDirtyFlagOffset = 2*kPointerSize;
On 2010/05/18 08:28:50, Mads Ager wrote:
> Space around binary op.

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode244
src/spaces.h:244: static const int kRegionSize      = 1 << kRegionSizeLog2;
On 2010/05/18 08:28:50, Mads Ager wrote:
> I would remove the extra spacing here.

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode258
src/spaces.h:258: // To avoid additional WATERMARK_INVALIDATED flag clearing
pass during
On 2010/05/17 17:48:53, iposva wrote:
> avoid an additional

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode259
src/spaces.h:259: // scavenge we just invalidate watermark on each old space
page after
On 2010/05/17 17:48:53, iposva wrote:
> invalidate the watermark

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode260
src/spaces.h:260: // processing it. And then we flip meaning of the
WATERMARK_INVALIDATED flag
On 2010/05/17 17:48:53, iposva wrote:
> flip the meaning

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode261
src/spaces.h:261: // at the begging of the next scavenge and each page becomes
marked as having
On 2010/05/17 17:48:53, iposva wrote:
> begging ?-> beginning

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode262
src/spaces.h:262: // valid watermark.
On 2010/05/17 17:48:53, iposva wrote:
> a valid watermark

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode265
src/spaces.h:265: // Returns true if page allocation watermark was not altered
during scavenge.
On 2010/05/17 17:48:53, iposva wrote:
> if the page

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode286
src/spaces.h:286: // This field contains meaning of WATERMARK_INVALIDATED flag.
On 2010/05/17 17:48:53, iposva wrote:
> contains the meaning of the WATERMARK_INVALIDATED flag

Done.

http://codereview.chromium.org/2101002/diff/2001/3027#newcode288
src/spaces.h:288: // it's meaning at the beginning of scavenge.
On 2010/05/17 17:48:53, iposva wrote:
> of a scavenge

Done.

http://codereview.chromium.org/2101002/diff/2001/3029
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2101002/diff/2001/3029#newcode4553
src/x64/codegen-x64.cc:4553: __ SmiToInteger32(rbx, rbx);
On 2010/05/17 15:14:23, antonm wrote:
> now this part of code probably could be simplified---if I remember it
correctly
> I needed two registers precisely for sometimes it is more convenient to work
> with smis and sometimes with ints.  But I am not 100% sure.
> 
> Could you file a bug and assign it to me when this CL is committed?

Will do.

[Mads Ager (chromium), 2010-05-20 11:19:08]

LGTM!

http://codereview.chromium.org/2101002/diff/26001/27005
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/2101002/diff/26001/27005#newcode117
src/arm/macro-assembler-arm.h:117: // For page containing |object| mark region
covering [object+offset] dirty.
page -> the page
region -> the region

http://codereview.chromium.org/2101002/diff/26001/27005#newcode120
src/arm/macro-assembler-arm.h:120: // For page containing |object| mark region
covering [object+offset] dirty.
page -> the page
region -> the region

http://codereview.chromium.org/2101002/diff/26001/27005#newcode121
src/arm/macro-assembler-arm.h:121: // Object address must be in the first 8K of
an allocated page.
The object address

http://codereview.chromium.org/2101002/diff/26001/27006
File src/builtins.cc (right):

http://codereview.chromium.org/2101002/diff/26001/27006#newcode308
src/builtins.cc:308: // region dirty marks.
There is a mix of 'page dirty marks' and 'region dirty marks' in these comments.
Choose one?

http://codereview.chromium.org/2101002/diff/26001/27010
File src/heap.cc (right):

http://codereview.chromium.org/2101002/diff/26001/27010#newcode823
src/heap.cc:823: // allocation to avoid overheads. So to maintain watermark
invariant
overheads -> overhead
watermark invariant -> the watermark invariant

http://codereview.chromium.org/2101002/diff/26001/27010#newcode824
src/heap.cc:824: // we have to manually cache watermark and mark page as having
invalid
the watermark
the page
an invalid

http://codereview.chromium.org/2101002/diff/26001/27010#newcode3537
src/heap.cc:3537: region_end   = region_start + Page::kRegionSize;
Remove extra indentation.

http://codereview.chromium.org/2101002/diff/26001/27010#newcode3547
src/heap.cc:3547: region_end   = region_start + Page::kRegionSize;
Ditto.

http://codereview.chromium.org/2101002/diff/26001/27010#newcode3549
src/heap.cc:3549: mask  <<= 1;
Two spaces before <<=

http://codereview.chromium.org/2101002/diff/26001/27011
File src/heap.h (right):

http://codereview.chromium.org/2101002/diff/26001/27011#newcode1331
src/heap.h:1331: // Visitor class to verify interior pointers in spaces that
does not contain
does -> do

http://codereview.chromium.org/2101002/diff/26001/27016
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/2101002/diff/26001/27016#newcode132
src/ia32/macro-assembler-ia32.cc:132: // array access: calculate the destination
address in the same manner as
array -> Array