Partially fix semantics of Array.push()

Semantics of elements accessors are now preserved in all optimized code paths
through Array.push(). Previously it was possible to have inconsistent behavior
between optimized and unoptimized code, and there were cases where element
accessors were completely ingored.

R=verwaest@chromium.org

Review URL: https://codereview.chromium.org/232873002

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@20655 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/builtins.cc

@@ -301,6 +301,11 @@ static inline MaybeHandle<FixedArrayBase> EnsureJSArrayWithWritableFastElements(
     int first_added_arg) {
   if (!receiver->IsJSArray()) return MaybeHandle<FixedArrayBase>();
   Handle<JSArray> array = Handle<JSArray>::cast(receiver);
+  // If there may be elements accessors in the prototype chain, the fast path
+  // cannot be used.
+  if (array->map()->DictionaryElementsInPrototypeChainOnly()) {
+    return MaybeHandle<FixedArrayBase>();
+  }
   if (array->map()->is_observed()) return MaybeHandle<FixedArrayBase>();
   if (!array->map()->is_extensible()) return MaybeHandle<FixedArrayBase>();
   Handle<FixedArrayBase> elms(array->elements(), isolate);

src/hydrogen.cc

@@ -6781,9 +6781,12 @@ HInstruction* HGraphBuilder::BuildConstantMapCheck(Handle<JSObject> constant,
 
 HInstruction* HGraphBuilder::BuildCheckPrototypeMaps(Handle<JSObject> prototype,
                                                      Handle<JSObject> holder) {
-  while (!prototype.is_identical_to(holder)) {
+  while (holder.is_null() || !prototype.is_identical_to(holder)) {
     BuildConstantMapCheck(prototype, top_info());
-    prototype = handle(JSObject::cast(prototype->GetPrototype()));
+    Object* next_prototype = prototype->GetPrototype();
+    if (next_prototype->IsNull()) return NULL;
+    CHECK(next_prototype->IsJSObject());
+    prototype = handle(JSObject::cast(next_prototype));
   }
 
   HInstruction* checked_object = BuildConstantMapCheck(prototype, top_info());
@@ -7661,6 +7664,17 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       ElementsKind elements_kind = receiver_map->elements_kind();
       if (!IsFastElementsKind(elements_kind)) return false;
 
+      // If there may be elements accessors in the prototype chain, the fast
+      // inlined version can't be used.
+      if (receiver_map->DictionaryElementsInPrototypeChainOnly()) return false;
+      // If there currently can be no elements accessors on the prototype chain,
+      // it doesn't mean that there won't be any later. Install a full prototype
+      // chain check to trap element accessors being installed on the prototype
+      // chain, which would cause elements to go to dictionary mode and result
+      // in a map change.
+      Handle<JSObject> prototype(JSObject::cast(receiver_map->prototype()));
+      BuildCheckPrototypeMaps(prototype, Handle<JSObject>());
+
       HValue* op_vals[] = {
         context(),
         // Receiver.

src/string.js

@@ -658,7 +658,7 @@ function StringSplitOnRegExp(subject, separator, limit, length) {
   var currentIndex = 0;
   var startIndex = 0;
   var startMatch = 0;
-  var result = [];
+  var result = new InternalArray();
 
   outer_loop:
   while (true) {
@@ -703,7 +703,9 @@ function StringSplitOnRegExp(subject, separator, limit, length) {
 
     startIndex = currentIndex = endIndex;
   }
-  return result;
+  var array_result = [];
+  %MoveArrayContents(result, array_result);
+  return array_result;
 }
 
 

test/mjsunit/array-push2.js

+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+var array = [];
+var v = 0;
+
+Object.defineProperty(Array.prototype, "0", {
+  get: function() { return "get " + v; },
+  set: function(value) { v += value; }
+});
+
+array[0] = 10;
+assertEquals(0, array.length);
+assertEquals(10, v);
+assertEquals("get 10", array[0]);
+
+array.push(100);
+assertEquals(1, array.length);
+assertEquals(110, v);
+assertEquals("get 110", array[0]);

test/mjsunit/array-push3.js

+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+var array = [];
+
+function push(array, value) {
+  array.push(value);
+}
+
+push(array, 0);
+push(array, 1);
+push(array, 2);
+%OptimizeFunctionOnNextCall(push);
+push(array, 3);
+
+var v = 0;
+Object.defineProperty(Array.prototype, "4", {
+  get: function() { return 100; },
+  set: function(value) { v = value; }
+});
+
+push(array, 4);
+
+assertEquals(5, array.length);
+assertEquals(100, array[4]);
+assertEquals(4, v);

test/mjsunit/array-push4.js

+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+var v = 0;
+var my_array_proto = {};
+my_array_proto.__proto__ = [].__proto__;
+Object.defineProperty(my_array_proto, "0", {
+get: function() { return "get " + v; },
+set: function(value) { v += value; }
+});
+
+
+// Test that element accessors are called in standard push cases.
+array = [];
+array.__proto__ = my_array_proto;
+
+array[0] = 10;
+assertEquals(0, array.length);
+assertEquals(10, v);
+assertEquals("get 10", array[0]);
+
+Array.prototype.push.call(array, 100);
+assertEquals(1, array.length);
+assertEquals(110, v);
+assertEquals("get 110", array[0]);
+
+array = [];
+array.__proto__ = my_array_proto;
+
+assertEquals(0, array.length);
+array.push(110);
+assertEquals(1, array.length);
+assertEquals(220, v);
+assertEquals("get 220", array[0]);
+
+// Test that elements setters/getters on prototype chain are property detected
+// and don't lead to overzealous optimization.
+v = 0;
+function push_wrapper_1(array, value) {
+  array.push(value);
+}
+array = [];
+array.__proto__ = my_array_proto;
+push_wrapper_1(array, 100);
+assertEquals(1, array.length);
+assertEquals(100, v);
+push_wrapper_1(array, 100);
+assertEquals(2, array.length);
+assertEquals(100, v);
+assertEquals("get 100", array[0]);
+%OptimizeFunctionOnNextCall(push_wrapper_1);
+array = [];
+array.__proto__ = my_array_proto;
+push_wrapper_1(array, 100);
+assertEquals(1, array.length);
+assertEquals(200, v);
+assertEquals("get 200", array[0]);

test/mjsunit/array-push5.js

+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+var v = 0;
+
+// Test that elements setters/getters on prototype chain set after the fact are
+// property detected and don't lead to overzealous optimization.
+var my_array_proto = {};
+my_array_proto.__proto__ = [].__proto__;
+
+function push_wrapper_2(array, value) {
+  array.push(value);
+}
+array = [];
+array.__proto__ = my_array_proto;
+push_wrapper_2(array, 66);
+assertEquals(1, array.length);
+assertEquals(0, v);
+assertEquals(66, array[0]);
+push_wrapper_2(array, 77);
+assertEquals(2, array.length);
+assertEquals(0, v);
+assertEquals(77, array[1]);
+%OptimizeFunctionOnNextCall(push_wrapper_2);
+push_wrapper_2(array, 88);
+assertEquals(3, array.length);
+assertEquals(0, v);
+assertEquals(88, array[2]);
+assertOptimized(push_wrapper_2);
+// Defining accessor should deopt optimized push.
+Object.defineProperty(my_array_proto, "3", {
+get: function() { return "get " + v; },
+set: function(value) { v += value; }
+});
+assertUnoptimized(push_wrapper_2);
+push_wrapper_2(array, 99);
+assertEquals(4, array.length);
+assertEquals(99, v);
+assertEquals("get 99", array[3]);