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#newcode20
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#newcode555
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#newcode658
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".
https://codereview.chromium.org/391693002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
