Reviewers: Toon Verwaest,
Message:
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#newcode20
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#newcode555
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#newcode658
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.
Description:
In-object double fields unboxing (for x64 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.
Please review this at https://codereview.chromium.org/391693002/
Base URL: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+1186, -204 lines):
M src/bootstrapper.cc
M src/code-stubs-hydrogen.cc
M src/field-index.h
M src/flag-definitions.h
M src/globals.h
M src/heap/heap.cc
M src/heap/mark-compact.cc
M src/heap/objects-visiting.h
M src/heap/objects-visiting.cc
M src/heap/store-buffer.cc
M src/hydrogen.cc
M src/hydrogen-instructions.h
M src/json-stringifier.h
M src/objects.h
M src/objects.cc
M src/objects-debug.cc
M src/objects-inl.h
M src/objects-printer.cc
M src/property-details.h
M src/runtime/runtime-object.cc
M src/string-stream.cc
M src/x64/lithium-codegen-x64.cc
M test/cctest/cctest.gyp
M test/cctest/test-heap.cc
A test/cctest/test-unboxed-doubles.cc
--
--
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.
