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-visiting.h
File src/heap/objects-visiting.h (right):
https://codereview.chromium.org/391693002/diff/160001/src/heap/objects-visiting.h#newcode238
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.cc#newcode523
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#newcode20
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#newcode2082
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.cc#newcode2389
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.cc#newcode2397
test/cctest/test-heap.cc:2397: }
Same here.
https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxed-doubles.cc
File test/cctest/test-unboxed-doubles.cc (right):
https://codereview.chromium.org/391693002/diff/160001/test/cctest/test-unboxed-doubles.cc#newcode663
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-unboxed-doubles.cc#newcode687
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.
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.
