Feedback addressed, landing.
http://codereview.chromium.org/9310117/diff/13029/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/arm/codegen-arm.cc#newcode100
src/arm/codegen-arm.cc:100: __ cmp(r4, r5);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot(r4, Heap::kEmptyFixedArrayRootIndex) ?
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/codegen-arm.cc#newcode126
src/arm/codegen-arm.cc:126: EMIT_REMEMBERED_SET,
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
OMIT_REMEMBERED_SET (maps are never in new space).
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/codegen-arm.cc#newcode163
src/arm/codegen-arm.cc:163: EMIT_REMEMBERED_SET,
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
OMIT_REMEMBERED_SET (maps are never in new space).
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/codegen-arm.cc#newcode235
src/arm/codegen-arm.cc:235: __ cmp(r4, r5);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot?
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/codegen-arm.cc#newcode329
src/arm/codegen-arm.cc:329: EMIT_REMEMBERED_SET,
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
OMIT_
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc#newcode4210
src/arm/stub-cache-arm.cc:4210: __ LoadRoot(scratch,
Heap::kHeapNumberMapRootIndex);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot?
also root index is incorrect. should be empty fixed array.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc#newcode4242
src/arm/stub-cache-arm.cc:4242: __ add(length_reg, length_reg,
Operand(Smi::FromInt(1)));
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
can write directly 1 into the length field.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc#newcode4353
src/arm/stub-cache-arm.cc:4353: __ cmp(scratch1, scratch2);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc#newcode4364
src/arm/stub-cache-arm.cc:4364: __ cmp(elements_reg, scratch1);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
also root index is incorrect. should be empty fixed array.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc#newcode4384
src/arm/stub-cache-arm.cc:4384: __ str(elements_reg,
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
elements of the backing store are completely uninitialized; if this is
intentional please add comment.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/arm/stub-cache-arm.cc#newcode4391
src/arm/stub-cache-arm.cc:4391: __ add(length_reg, length_reg,
Operand(Smi::FromInt(1)));
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
store 1 directly without addition.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/9310117/diff/13029/src/code-stubs.h#newcode990
src/code-stubs.h:990: : is_js_array_(is_js_array),
On 2012/02/09 14:51:25, Jakob wrote:
nit: 4-space indent
Done.
http://codereview.chromium.org/9310117/diff/13029/src/code-stubs.h#newcode996
src/code-stubs.h:996: int multiplier = is_js_array_ ? 1 : 0;
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
is there any particular reason why you don't want to use BitField's
here? I
think it would be much more readable.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/code-stubs.h#newcode1103
src/code-stubs.h:1103: StrictModeBits::encode(strict_mode_);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
i think grow_mode_ should be embedded into MinorKey.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/ia32/codegen-ia32.cc#newcode422
src/ia32/codegen-ia32.cc:422: EMIT_REMEMBERED_SET,
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
map can not be in new space.
We only need an incremental write barrier here.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/ia32/codegen-ia32.cc#newcode474
src/ia32/codegen-ia32.cc:474: EMIT_REMEMBERED_SET,
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
ditto
Done.
http://codereview.chromium.org/9310117/diff/13029/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/ia32/stub-cache-ia32.cc#newcode3776
src/ia32/stub-cache-ia32.cc:3776: if (is_js_array && grow_mode ==
ALLOW_JSARRAY_GROWTH) {
It turns out the existing elements ICs are very broken WRT honoring
setters, even if they are set on the prototype before the IC is used.
This CL doesn't make it any worse, but this is totally something we need
to fix.
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
We might have setters on the prototype:
function add(x) { x[x.length] = x.length; }
Array.prototype.__defineSetter__("1", function (v) {
print("I am setter!");
});
var x = new Array(0);
add(x);
add(x);
I think this setter will fire without this CL (which is correct) but
will not
fire with this CL. This seems like yet another "setter on the
prototype" compat
problem.
http://codereview.chromium.org/9310117/diff/13029/src/ia32/stub-cache-ia32.cc#newcode3823
src/ia32/stub-cache-ia32.cc:3823: __ add(FieldOperand(edx,
JSArray::kLengthOffset),
On 2012/02/09 16:29:19, Vyacheslav Egorov wrote:
can be actually move of smi 1 instead of addition as length is known
to be 0.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/ia32/stub-cache-ia32.cc#newcode3957
src/ia32/stub-cache-ia32.cc:3957: __ mov(FieldOperand(edx,
JSObject::kElementsOffset), edi);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
elements of the backing store are completely uninitialized; if this is
intentional please add comment.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/objects-debug.cc
File src/objects-debug.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/objects-debug.cc#newcode284
src/objects-debug.cc:284: || map()->has_fast_smi_only_elements() ||
On 2012/02/09 14:51:25, Jakob wrote:
nit: all operators at the end of the line, please.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/9310117/diff/13029/src/objects.h#newcode4226
src/objects.h:4226: static inline StrictModeFlag
GetStrictMode(ExtraICState extra_ic_state) {
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
Consider using BitField to encode and decode extra_ic_state.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/stub-cache.h
File src/stub-cache.h (right):
http://codereview.chromium.org/9310117/diff/13029/src/stub-cache.h#newcode707
src/stub-cache.h:707: PropertyType type,
On 2012/02/09 14:51:25, Jakob wrote:
Why this formatting change?
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/codegen-x64.cc
File src/x64/codegen-x64.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/x64/codegen-x64.cc#newcode190
src/x64/codegen-x64.cc:190: __ Cmp(r8,
masm->isolate()->factory()->empty_fixed_array());
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/codegen-x64.cc#newcode257
src/x64/codegen-x64.cc:257: EMIT_REMEMBERED_SET,
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
OMIT_
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/codegen-x64.cc#newcode303
src/x64/codegen-x64.cc:303: __ Cmp(r8,
masm->isolate()->factory()->empty_fixed_array());
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/codegen-x64.cc#newcode394
src/x64/codegen-x64.cc:394: EMIT_REMEMBERED_SET,
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
OMIT
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3562
src/x64/stub-cache-x64.cc:3562: __ Cmp(rdi,
masm->isolate()->factory()->empty_fixed_array());
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3582
src/x64/stub-cache-x64.cc:3582: __ Move(rbx,
masm->isolate()->factory()->the_hole_value());
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
LoadRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3596
src/x64/stub-cache-x64.cc:3596: __ SmiAddConstant(FieldOperand(rdx,
JSArray::kLengthOffset),
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
Store 1 directly into field instead of addition.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3602
src/x64/stub-cache-x64.cc:3602: __ Cmp(FieldOperand(rdi,
HeapObject::kMapOffset),
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3693
src/x64/stub-cache-x64.cc:3693: __ Cmp(FieldOperand(rax,
HeapObject::kMapOffset),
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3701
src/x64/stub-cache-x64.cc:3701: __ Cmp(rdi,
masm->isolate()->factory()->empty_fixed_array());
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
CompareRoot
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3718
src/x64/stub-cache-x64.cc:3718:
masm->isolate()->factory()->fixed_double_array_map());
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
indentation
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3720
src/x64/stub-cache-x64.cc:3720:
Smi::FromInt(JSArray::kPreallocatedArrayElements));
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
indentation
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3723
src/x64/stub-cache-x64.cc:3723: __ movq(FieldOperand(rdx,
JSObject::kElementsOffset), rdi);
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
elements seem to be uninitialized. if this is intentional please add a
comment.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3728
src/x64/stub-cache-x64.cc:3728: __ SmiAddConstant(FieldOperand(rdx,
JSArray::kLengthOffset),
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
Store 1 directly instead of add.
Done.
http://codereview.chromium.org/9310117/diff/13029/src/x64/stub-cache-x64.cc#newcode3728
src/x64/stub-cache-x64.cc:3728: __ SmiAddConstant(FieldOperand(rdx,
JSArray::kLengthOffset),
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
Store 1 directly instead of add.
Done.
http://codereview.chromium.org/9310117/diff/13029/test/mjsunit/array-store-and-grow.js
File test/mjsunit/array-store-and-grow.js (right):
http://codereview.chromium.org/9310117/diff/13029/test/mjsunit/array-store-and-grow.js#newcode27
test/mjsunit/array-store-and-grow.js:27:
On 2012/02/10 00:19:18, Vyacheslav Egorov wrote:
consider adding test that checks COW handling. like
function makeCOW() { return [1,2,3]; }
var cow1 = makeCOW(), cow2 = makeCOW();
store(cow1);
check_that_cow_is_fine(cow2);
Done.
http://codereview.chromium.org/9310117/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
