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.
http://codereview.chromium.org/2101002/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
