In-object double fields unboxing (for 64-bit only).

In-object double fields unboxing (for 64-bit only).

This CL introduces LayoutDescriptor which is responsible for tracking which in-object fields are tagged and which are not.
LayoutDescriptor field added to Map. Currently unboxing is disabled.

R=hpayer@chromium.org, verwaest@chromium.org

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

[Igor Sheludko, 2014-07-14 11:25:22]

PTAL

[Toon Verwaest, 2014-07-29 15:02:09]

https://codereview.chromium.org/391693002/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/heap.cc#newcode2341
src/heap.cc:2341: // All initial maps have only tagged fields.
// Initialize to only containing tagged fields.

https://codereview.chromium.org/391693002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/mark-compact.cc#newcode2849
src/mark-compact.cc:2849: || helper.IsTagged(src_slot - src_addr)
#if V8_DOUBLE_FIELDS_UNBOXING
 if (.. || ) {
#else
 if (..) {
#endif

https://codereview.chromium.org/391693002/diff/1/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/objects-debug.cc#newcode266
src/objects-debug.cc:266: if (map()->IsUnboxedDoubleField(index)) continue;
Assert that r.IsDouble()

https://codereview.chromium.org/391693002/diff/1/src/objects-debug.cc#newcode308
src/objects-debug.cc:308: ASSERT(!FLAG_unbox_double_fields ||
SLOW_ASSERT?

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode2049
src/objects-inl.h:2049: return isolate->factory()->NewHeapNumber(value,
MUTABLE);
Should not call this for unboxed doubles.
We should not return Mutable heapnumber for these cases as it will force
reboxing.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode2071
src/objects-inl.h:2071: if (map->IsUnboxedDoubleField(index)) {
Always go through FastDoublePropertyAtPut?
I wouldn't support unboxed doubles fields in this method.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode2812
src/objects-inl.h:2812: if (FLAG_unbox_double_fields)
drop_cached_layout_descriptor();
This doesn't belong here

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode3124
src/objects-inl.h:3124: if (FLAG_unbox_double_fields)
drop_cached_layout_descriptor();
This doesn't belong here

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode3135
src/objects-inl.h:3135: if (FLAG_unbox_double_fields)
drop_cached_layout_descriptor();
This doesn't belong here

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode3308
src/objects-inl.h:3308: LayoutDescriptor* LayoutDescriptor::OptimizeFor(Map*
map) {
You shouldn't need to "undo" installing an expensive layout descriptor; just
don't install it in the first place.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode5393
src/objects-inl.h:5393: void Map::RebuildLayoutDescriptor(Handle<Map> map) {
Try to avoid introducing this method

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode5408
src/objects-inl.h:5408: set_layout_descriptor(layout_desc->OptimizeFor(this));
Only overwrite the layout descriptor if the current descriptor is not a smi;
otherwise it is already "optimal".

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode5422
src/objects-inl.h:5422: set_layout_descriptor(layout_desc->OptimizeFor(this));
Make sure the layout_desc that comes into this function is already optimal for
this map. It's being created for this map, so we can know up front what the
optimal format is.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode7430
src/objects-inl.h:7430: v->VisitPointers(HeapObject::RawField(object, offset),
VisitPointer?

https://codereview.chromium.org/391693002/diff/1/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/objects-visiting.cc#newc...
src/objects-visiting.cc:20: int instance_type, int instance_size, bool
has_non_tagged_fields) {
has_unboxed_fields?

https://codereview.chromium.org/391693002/diff/1/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/391693002/diff/1/src/objects.cc#oldcode7323
src/objects.cc:7323:
new_map->InitializeDescriptors(map->instance_descriptors());
This is overkill, set_instance_descriptors should be good enough. The length
didn't change.

Use UpdateDescriptors(DescriptorArray*, LayoutDescriptor*) instead.

https://codereview.chromium.org/391693002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/objects.cc#newcode3485
src/objects.cc:3485: if (!map()->IsUnboxedDoubleField(index) &&
Unnecessary by now

https://codereview.chromium.org/391693002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/391693002/diff/1/src/objects.h#newcode6454
src/objects.h:6454: inline static void RebuildLayoutDescriptor(Handle<Map> map);
Avoid having a Rebuild method. Just make sure that the map is always in a
consistent state. So update on append if necessary. However, for bootstrapper
related objects we can't do field type tracking anyway, so no unboxing is
necessary.

https://codereview.chromium.org/391693002/diff/1/src/objects.h#newcode6455
src/objects.h:6455: inline void InitializeDescriptors(DescriptorArray*
descriptors,
UpdateDescriptors / SetDescriptors

https://codereview.chromium.org/391693002/diff/1/src/objects.h#newcode6457
src/objects.h:6457: inline void InitializeOwnDescriptors(DescriptorArray*
descriptors,
InitializeDescriptors

https://codereview.chromium.org/391693002/diff/1/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/x64/stub-cache-x64.cc#ne...
src/x64/stub-cache-x64.cc:555: FieldIndex index =
FieldIndex::ForDescriptor(*transition, descriptor);
Nice cleanup. What about porting this to other platforms as well?

https://codereview.chromium.org/391693002/diff/1/src/x64/stub-cache-x64.cc#ne...
src/x64/stub-cache-x64.cc:658: Operand operand = FieldOperand(scratch1,
HeapNumber::kValueOffset);
Arg this code is confusing. Can you please just set up Operand after loading
values into scratch1? e.g.:

Operand operand = FLAG_unbox_double_fields && index.is_inobject()
    ? FieldOperand(receiver_reg, index.offset()
    : FieldOperand(scratch1, HeapNumber::kValueOffset);

I know it doesn't really matter, but it's a bit confusing that you "load
something from scratch1", and only then set up "scratch1".

[Igor Sheludko, 2014-10-30 14:23:44]

PTAL (I changed almost third half of the CL)

https://codereview.chromium.org/391693002/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/heap.cc#newcode2341
src/heap.cc:2341: // All initial maps have only tagged fields.
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> // Initialize to only containing tagged fields.

Done.

https://codereview.chromium.org/391693002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/mark-compact.cc#newcode2849
src/mark-compact.cc:2849: || helper.IsTagged(src_slot - src_addr)
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> #if V8_DOUBLE_FIELDS_UNBOXING
>  if (.. || ) {
> #else
>  if (..) {
> #endif

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/objects-debug.cc#newcode266
src/objects-debug.cc:266: if (map()->IsUnboxedDoubleField(index)) continue;
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> Assert that r.IsDouble()

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-debug.cc#newcode308
src/objects-debug.cc:308: ASSERT(!FLAG_unbox_double_fields ||
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> SLOW_ASSERT?

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode2049
src/objects-inl.h:2049: return isolate->factory()->NewHeapNumber(value,
MUTABLE);
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> Should not call this for unboxed doubles.
> We should not return Mutable heapnumber for these cases as it will force
> reboxing.

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode2071
src/objects-inl.h:2071: if (map->IsUnboxedDoubleField(index)) {
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> Always go through FastDoublePropertyAtPut?
> I wouldn't support unboxed doubles fields in this method.

I fixed callers of this methods where it makes sense (to avoid reboxing), but
there are other usages where we have to do exactly what this method does. So I
would prefer to keep this method to avoid code duplication.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode2812
src/objects-inl.h:2812: if (FLAG_unbox_double_fields)
drop_cached_layout_descriptor();
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> This doesn't belong here

Done. I removed the whole layout descriptor cache from descriptor array.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode3124
src/objects-inl.h:3124: if (FLAG_unbox_double_fields)
drop_cached_layout_descriptor();
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> This doesn't belong here

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode3135
src/objects-inl.h:3135: if (FLAG_unbox_double_fields)
drop_cached_layout_descriptor();
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> This doesn't belong here

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode3308
src/objects-inl.h:3308: LayoutDescriptor* LayoutDescriptor::OptimizeFor(Map*
map) {
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> You shouldn't need to "undo" installing an expensive layout descriptor; just
> don't install it in the first place.

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode5393
src/objects-inl.h:5393: void Map::RebuildLayoutDescriptor(Handle<Map> map) {
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> Try to avoid introducing this method

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode5408
src/objects-inl.h:5408: set_layout_descriptor(layout_desc->OptimizeFor(this));
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> Only overwrite the layout descriptor if the current descriptor is not a smi;
> otherwise it is already "optimal".

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode5422
src/objects-inl.h:5422: set_layout_descriptor(layout_desc->OptimizeFor(this));
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> Make sure the layout_desc that comes into this function is already optimal for
> this map. It's being created for this map, so we can know up front what the
> optimal format is.

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-inl.h#newcode7430
src/objects-inl.h:7430: v->VisitPointers(HeapObject::RawField(object, offset),
On 2014/07/29 15:02:08, Toon Verwaest wrote:
> VisitPointer?

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/objects-visiting.cc#newc...
src/objects-visiting.cc:20: int instance_type, int instance_size, bool
has_non_tagged_fields) {
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> has_unboxed_fields?

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/391693002/diff/1/src/objects.cc#oldcode7323
src/objects.cc:7323:
new_map->InitializeDescriptors(map->instance_descriptors());
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> This is overkill, set_instance_descriptors should be good enough. The length
> didn't change.
> 
> Use UpdateDescriptors(DescriptorArray*, LayoutDescriptor*) instead.

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/objects.cc#newcode3485
src/objects.cc:3485: if (!map()->IsUnboxedDoubleField(index) &&
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> Unnecessary by now

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/391693002/diff/1/src/objects.h#newcode6455
src/objects.h:6455: inline void InitializeDescriptors(DescriptorArray*
descriptors,
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> UpdateDescriptors / SetDescriptors

Done.

https://codereview.chromium.org/391693002/diff/1/src/objects.h#newcode6457
src/objects.h:6457: inline void InitializeOwnDescriptors(DescriptorArray*
descriptors,
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> InitializeDescriptors

Done.

https://codereview.chromium.org/391693002/diff/1/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/391693002/diff/1/src/x64/stub-cache-x64.cc#ne...
src/x64/stub-cache-x64.cc:555: FieldIndex index =
FieldIndex::ForDescriptor(*transition, descriptor);
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> Nice cleanup. What about porting this to other platforms as well?

Done.

https://codereview.chromium.org/391693002/diff/1/src/x64/stub-cache-x64.cc#ne...
src/x64/stub-cache-x64.cc:658: Operand operand = FieldOperand(scratch1,
HeapNumber::kValueOffset);
On 2014/07/29 15:02:09, Toon Verwaest wrote:
> Arg this code is confusing. Can you please just set up Operand after loading
> values into scratch1? e.g.:
> 
> Operand operand = FLAG_unbox_double_fields && index.is_inobject()
>     ? FieldOperand(receiver_reg, index.offset()
>     : FieldOperand(scratch1, HeapNumber::kValueOffset);
> 
> I know it doesn't really matter, but it's a bit confusing that you "load
> something from scratch1", and only then set up "scratch1".

Done.

[Toon Verwaest, 2014-10-31 16:23:41]

Looking good to me.

https://codereview.chromium.org/391693002/diff/80001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/flag-definitions.h#n...
src/flag-definitions.h:934: "enable in-object double fields unboxing (X64
only)")
64-bit only

https://codereview.chromium.org/391693002/diff/80001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/globals.h#newcode90
src/globals.h:90: #define V8_DOUBLE_FIELDS_UNBOXING 0
?

https://codereview.chromium.org/391693002/diff/80001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/objects-inl.h#newcod...
src/objects-inl.h:3202: 
Add a comment why this is a good idea (not wasting space, beneficial when
appending new descriptors).

https://codereview.chromium.org/391693002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/objects.h#newcode4832
src/objects.h:4832: class LayoutDescriptor : public
FixedTypedArray<Uint32ArrayTraits> {
What about putting this in a separate file?

[Igor Sheludko, 2014-11-03 12:45:49]

ishell@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Igor Sheludko, 2014-11-03 12:47:23]

I added LayoutDescriptor tests and addressed all Toon's comments, except
extracting LayoutDescriptor to another file. I'll do that in a following patch
set.

https://codereview.chromium.org/391693002/diff/80001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/flag-definitions.h#n...
src/flag-definitions.h:934: "enable in-object double fields unboxing (X64
only)")
On 2014/10/31 16:23:41, Toon Verwaest wrote:
> 64-bit only

Done.

https://codereview.chromium.org/391693002/diff/80001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/globals.h#newcode90
src/globals.h:90: #define V8_DOUBLE_FIELDS_UNBOXING 0
On 2014/10/31 16:23:41, Toon Verwaest wrote:
> ?

I'm going to land this in disabled mode and enable it in a next CL.

https://codereview.chromium.org/391693002/diff/80001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/80001/src/objects-inl.h#newcod...
src/objects-inl.h:3202: 
On 2014/10/31 16:23:41, Toon Verwaest wrote:
> Add a comment why this is a good idea (not wasting space, beneficial when
> appending new descriptors).

Done.

[Toon Verwaest, 2014-11-04 10:04:51]

https://codereview.chromium.org/391693002/diff/120001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/391693002/diff/120001/src/heap/objects-visiti...
src/heap/objects-visiting.h:109: bool has_unboxed_fields);
Use enum rather than bool

https://codereview.chromium.org/391693002/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/391693002/diff/120001/src/objects.cc#newcode8120
src/objects.cc:8120: if (layout_descriptor_length <= field_index +
field_width_in_words - 1) {
layout_descriptor_length = Max(layout_descriptor_length, field_index +
field_width_in_words)

https://codereview.chromium.org/391693002/diff/120001/src/objects.cc#newcode8144
src/objects.cc:8144: layout_descriptor =
layout_descriptor->SetTagged(field_index, false);
What about having a "SetDouble" or "SetRawData" rather than SetTagged(..false)

[Igor Sheludko, 2014-11-04 10:29:54]

ishell@chromium.org changed reviewers:
+ hpayer@chromium.org

[Igor Sheludko, 2014-11-04 10:46:29]

Moved LayoutDescriptor-related stuff to separate file and addressed last
comments.

https://codereview.chromium.org/391693002/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/391693002/diff/120001/src/objects.cc#newcode8120
src/objects.cc:8120: if (layout_descriptor_length <= field_index +
field_width_in_words - 1) {
On 2014/11/04 10:04:51, Toon Verwaest wrote:
> layout_descriptor_length = Max(layout_descriptor_length, field_index +
> field_width_in_words)

Done.

https://codereview.chromium.org/391693002/diff/120001/src/objects.cc#newcode8144
src/objects.cc:8144: layout_descriptor =
layout_descriptor->SetTagged(field_index, false);
On 2014/11/04 10:04:51, Toon Verwaest wrote:
> What about having a "SetDouble" or "SetRawData" rather than SetTagged(..false)

Done.

[Hannes Payer (out of office), 2014-11-06 12:29:46]

Cool stuff, already looking good. A few comments.

https://codereview.chromium.org/391693002/diff/160001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/globals.h#newcode90
src/globals.h:90: #define V8_DOUBLE_FIELDS_UNBOXING 0
wanna turn it on?

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
src/heap/objects-visiting.h:238: IterateRawPointers(heap, object, offset, offset
+ kPointerSize);
Instead of calling this for each field, do you think it has a performance
advantage to call it over maximum contiguous pointer ranges?
It is probably more readable how it is implemented right now.

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:523: FindPointersToNewSpaceInRegion(slot, slot +
kPointerSize,
Similar as before, do you think maximum contiguous pointer ranges make sense
here?

https://codereview.chromium.org/391693002/diff/160001/src/layout-descriptor.h
File src/layout-descriptor.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/layout-descriptor.h...
src/layout-descriptor.h:20: // field contains non tagged value and therefore
must be skipped by GC.
... *the* corresponding field contains *a* non-tagged value and ...

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h#newco...
src/objects-inl.h:2082: return
!map()->layout_descriptor()->IsTagged(index.property_index());
You could call IsUnboxedDoubleFrield from map.

https://codereview.chromium.org/391693002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/objects.h#newcode859
src/objects.h:859: class ObjectVisitor;
Can we fix the alphabetic order here. Move ObjectVisitor down.

https://codereview.chromium.org/391693002/diff/160001/src/objects.h#newcode6421
src/objects.h:6421: friend class TestAccessor;
Uff, do we really have to add test dependencies to the code?

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2389: }
Can we check the else case here, that we have a double value?

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2397: }
Same here.

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxe...
File test/cctest/test-unboxed-doubles.cc (right):

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxe...
test/cctest/test-unboxed-doubles.cc:663:
SimulateFullSpace(CcTest::heap()->old_pointer_space());
You don't need the SimulateFullSpace call.

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxe...
test/cctest/test-unboxed-doubles.cc:687:
CcTest::heap()->CollectAllGarbage(i::Heap::kNoGCFlags);
Please verify here that boom_value is still boom_value, i.e., the gc did not
update the value accidently.

[Igor Sheludko, 2014-11-07 08:03:53]

https://codereview.chromium.org/391693002/diff/160001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/globals.h#newcode90
src/globals.h:90: #define V8_DOUBLE_FIELDS_UNBOXING 0
On 2014/11/06 12:29:46, Hannes Payer wrote:
> wanna turn it on?

Yes, but in next CL. And maybe even in next Canary.

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
src/heap/objects-visiting.h:238: IterateRawPointers(heap, object, offset, offset
+ kPointerSize);
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Instead of calling this for each field, do you think it has a performance
> advantage to call it over maximum contiguous pointer ranges?
> It is probably more readable how it is implemented right now.

Good point! But let me do that in a next CL.

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:523: FindPointersToNewSpaceInRegion(slot, slot +
kPointerSize,
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Similar as before, do you think maximum contiguous pointer ranges make sense
> here?

I would prefer to implement that in a next CL.

https://codereview.chromium.org/391693002/diff/160001/src/layout-descriptor.h
File src/layout-descriptor.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/layout-descriptor.h...
src/layout-descriptor.h:20: // field contains non tagged value and therefore
must be skipped by GC.
On 2014/11/06 12:29:46, Hannes Payer wrote:
> ... *the* corresponding field contains *a* non-tagged value and ...

Done.

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h#newco...
src/objects-inl.h:2082: return
!map()->layout_descriptor()->IsTagged(index.property_index());
On 2014/11/06 12:29:46, Hannes Payer wrote:
> You could call IsUnboxedDoubleFrield from map.

I made two versions intentionally, to postpone call to map() to as late as
possible.

https://codereview.chromium.org/391693002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/objects.h#newcode859
src/objects.h:859: class ObjectVisitor;
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Can we fix the alphabetic order here. Move ObjectVisitor down.

Done.

https://codereview.chromium.org/391693002/diff/160001/src/objects.h#newcode6421
src/objects.h:6421: friend class TestAccessor;
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Uff, do we really have to add test dependencies to the code?

Done.

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2389: }
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Can we check the else case here, that we have a double value?

Done.

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2397: }
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Same here.

Done.

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxe...
File test/cctest/test-unboxed-doubles.cc (right):

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxe...
test/cctest/test-unboxed-doubles.cc:663:
SimulateFullSpace(CcTest::heap()->old_pointer_space());
On 2014/11/06 12:29:46, Hannes Payer wrote:
> You don't need the SimulateFullSpace call.

Done.

https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxe...
test/cctest/test-unboxed-doubles.cc:687:
CcTest::heap()->CollectAllGarbage(i::Heap::kNoGCFlags);
On 2014/11/06 12:29:46, Hannes Payer wrote:
> Please verify here that boom_value is still boom_value, i.e., the gc did not
> update the value accidently.

Good point! Done.

[Toon Verwaest, 2014-11-07 09:54:12]

lgtm

[Hannes Payer (out of office), 2014-11-10 15:26:08]

LGTM, with nits.

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
src/heap/objects-visiting.h:238: IterateRawPointers(heap, object, offset, offset
+ kPointerSize);
On 2014/11/07 08:03:52, Igor Sheludko wrote:
> On 2014/11/06 12:29:46, Hannes Payer wrote:
> > Instead of calling this for each field, do you think it has a performance
> > advantage to call it over maximum contiguous pointer ranges?
> > It is probably more readable how it is implemented right now.
> 
> Good point! But let me do that in a next CL.

Then please leave a todo.

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:523: FindPointersToNewSpaceInRegion(slot, slot +
kPointerSize,
On 2014/11/07 08:03:52, Igor Sheludko wrote:
> On 2014/11/06 12:29:46, Hannes Payer wrote:
> > Similar as before, do you think maximum contiguous pointer ranges make sense
> > here?
> 
> I would prefer to implement that in a next CL.

Then please leave a todo.

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h#newco...
src/objects-inl.h:2082: return
!map()->layout_descriptor()->IsTagged(index.property_index());
On 2014/11/07 08:03:52, Igor Sheludko wrote:
> On 2014/11/06 12:29:46, Hannes Payer wrote:
> > You could call IsUnboxedDoubleFrield from map.
> 
> I made two versions intentionally, to postpone call to map() to as late as
> possible.

You are already calling map here. You could call
map()->IsUnboxedDoubleField(index) instead of
!map()->layout_descriptor()->IsTagged(index.property_index());

[Igor Sheludko, 2014-11-10 15:43:54]

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiti...
src/heap/objects-visiting.h:238: IterateRawPointers(heap, object, offset, offset
+ kPointerSize);
On 2014/11/10 15:26:08, Hannes Payer wrote:
> On 2014/11/07 08:03:52, Igor Sheludko wrote:
> > On 2014/11/06 12:29:46, Hannes Payer wrote:
> > > Instead of calling this for each field, do you think it has a performance
> > > advantage to call it over maximum contiguous pointer ranges?
> > > It is probably more readable how it is implemented right now.
> > 
> > Good point! But let me do that in a next CL.
> 
> Then please leave a todo.

Done.

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/391693002/diff/160001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:523: FindPointersToNewSpaceInRegion(slot, slot +
kPointerSize,
On 2014/11/10 15:26:08, Hannes Payer wrote:
> On 2014/11/07 08:03:52, Igor Sheludko wrote:
> > On 2014/11/06 12:29:46, Hannes Payer wrote:
> > > Similar as before, do you think maximum contiguous pointer ranges make
sense
> > > here?
> > 
> > I would prefer to implement that in a next CL.
> 
> Then please leave a todo.

Done.

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/391693002/diff/160001/src/objects-inl.h#newco...
src/objects-inl.h:2082: return
!map()->layout_descriptor()->IsTagged(index.property_index());
On 2014/11/10 15:26:08, Hannes Payer wrote:
> On 2014/11/07 08:03:52, Igor Sheludko wrote:
> > On 2014/11/06 12:29:46, Hannes Payer wrote:
> > > You could call IsUnboxedDoubleFrield from map.
> > 
> > I made two versions intentionally, to postpone call to map() to as late as
> > possible.
> 
> You are already calling map here. You could call
> map()->IsUnboxedDoubleField(index) instead of
> !map()->layout_descriptor()->IsTagged(index.property_index());

Ok, lets make it look cleaner. Done.

[Igor Sheludko, 2014-11-10 16:35:03]

Committed patchset #12 (id:220001) manually as 25250 (presubmit successful).