LGTM with comments below addressed.
http://codereview.chromium.org/8372028/diff/1/src/arm/builtins-arm.cc File src/arm/builtins-arm.cc (right): http://codereview.chromium.org/8372028/diff/1/src/arm/builtins-arm.cc#newcode150 src/arm/builtins-arm.cc:150: // Fill the FixedArray with the hole value. Since initial_capacity can be zero, this should be: if (initial_capacity > 0) { // Fill the FixedArray with the hole value. ASSERT_EQ(2 * kPointerSize, FixedArray::kHeaderSize); __ LoadRoot(scratch3, Heap::kTheHoleValueRootIndex); for (int i = 0; i < initial_capacity; i++) { __ str(scratch3, MemOperand(scratch1, kPointerSize, PostIndex)); } } to avoid the useless load in the zero case. You might also consider a static assert that the initial capacity is small enough (say, not more than 10), with the comment that we should reconsider unrolling the store loop in the case that the initial capacity is large. http://codereview.chromium.org/8372028/diff/1/src/x64/builtins-x64.cc File src/x64/builtins-x64.cc (right): http://codereview.chromium.org/8372028/diff/1/src/x64/builtins-x64.cc#newcode1033 src/x64/builtins-x64.cc:1033: __ Move(FieldOperand(result, JSArray::kPropertiesOffset), While you're at this code, you could eliminate FACTORY by Factory* factory = masm->isolate()->factory(); http://codereview.chromium.org/8372028/diff/1/src/x64/builtins-x64.cc#newcode1066 src/x64/builtins-x64.cc:1066: __ Move(scratch3, FACTORY->the_hole_value()); I think __ LoadRoot(scratch3, Heap::kTheHoleValueRootIndex); is a smaller encoding of the load. http://codereview.chromium.org/8372028/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
