[runtime] Keep FAST_SLOPPY_ARGUMENTS packed

With this CL SloppyArguments immediately go to dictionary elements on
deletion, keeping the arguments backing store packed.

Bug: v8:6251
Change-Id: I90d1972179447bf6810e7fe2b8e0bc8703b38d9d
Reviewed-on: https://chromium-review.googlesource.com/486921Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45286}

src/elements.cc

@@ -3496,6 +3496,8 @@ class SloppyArgumentsElementsAccessor
     uint32_t entry = ArgumentsAccessor::GetEntryForIndexImpl(
         isolate, holder, arguments, index, filter);
     if (entry == kMaxUInt32) return kMaxUInt32;
+    // Arguments entries could overlap with the dictionary entries, hence offset
+    // them by the number of context mapped entries.
     return elements->parameter_map_length() + entry;
   }
 
@@ -3519,19 +3521,28 @@ class SloppyArgumentsElementsAccessor
   }
 
   static void DeleteImpl(Handle<JSObject> obj, uint32_t entry) {
-    SloppyArgumentsElements* elements =
-        SloppyArgumentsElements::cast(obj->elements());
+    Handle<SloppyArgumentsElements> elements(
+        SloppyArgumentsElements::cast(obj->elements()));
     uint32_t length = elements->parameter_map_length();
+    uint32_t delete_or_entry = entry;
+    if (entry < length) {
+      delete_or_entry = kMaxUInt32;
+    }
+    Subclass::SloppyDeleteImpl(obj, elements, delete_or_entry);
+    // SloppyDeleteImpl allocates a new dictionary elements store. For making
+    // heap verification happy we postpone clearing out the mapped entry.
     if (entry < length) {
-      // TODO(kmillikin): We could check if this was the last aliased
-      // parameter, and revert to normal elements in that case.  That
-      // would enable GC of the context.
       elements->set_mapped_entry(entry, obj->GetHeap()->the_hole_value());
-    } else {
-      Subclass::DeleteFromArguments(obj, entry - length);
     }
   }
 
+  static void SloppyDeleteImpl(Handle<JSObject> obj,
+                               Handle<SloppyArgumentsElements> elements,
+                               uint32_t entry) {
+    // Implemented in subclasses.
+    UNREACHABLE();
+  }
+
   static void CollectElementIndicesImpl(Handle<JSObject> object,
                                         Handle<FixedArrayBase> backing_store,
                                         KeyAccumulator* keys) {
@@ -3682,17 +3693,21 @@ class SlowSloppyArgumentsElementsAccessor
     }
     return result;
   }
-  static void DeleteFromArguments(Handle<JSObject> obj, uint32_t entry) {
+  static void SloppyDeleteImpl(Handle<JSObject> obj,
+                               Handle<SloppyArgumentsElements> elements,
+                               uint32_t entry) {
+    // No need to delete a context mapped entry from the arguments elements.
+    if (entry == kMaxUInt32) return;
     Isolate* isolate = obj->GetIsolate();
-    Handle<SloppyArgumentsElements> elements(
-        SloppyArgumentsElements::cast(obj->elements()), isolate);
     Handle<SeededNumberDictionary> dict(
         SeededNumberDictionary::cast(elements->arguments()), isolate);
     // TODO(verwaest): Remove reliance on index in Shrink.
     uint32_t index = GetIndexForEntryImpl(*dict, entry);
-    Handle<Object> result = SeededNumberDictionary::DeleteProperty(dict, entry);
+    int length = elements->parameter_map_length();
+    Handle<Object> result =
+        SeededNumberDictionary::DeleteProperty(dict, entry - length);
     USE(result);
-    DCHECK(result->IsTrue(dict->GetIsolate()));
+    DCHECK(result->IsTrue(isolate));
     Handle<FixedArray> new_elements =
         SeededNumberDictionary::Shrink(dict, index);
     elements->set_arguments(*new_elements);
@@ -3816,10 +3831,28 @@ class FastSloppyArgumentsElementsAccessor
     return FastHoleyObjectElementsAccessor::NormalizeImpl(object, arguments);
   }
 
-  static void DeleteFromArguments(Handle<JSObject> obj, uint32_t entry) {
-    Handle<FixedArray> arguments =
-        GetArguments(obj->GetIsolate(), obj->elements());
-    FastHoleyObjectElementsAccessor::DeleteCommon(obj, entry, arguments);
+  static Handle<SeededNumberDictionary> NormalizeArgumentsElements(
+      Handle<JSObject> object, Handle<SloppyArgumentsElements> elements,
+      uint32_t* entry) {
+    Handle<SeededNumberDictionary> dictionary =
+        JSObject::NormalizeElements(object);
+    elements->set_arguments(*dictionary);
+    // kMaxUInt32 indicates that a context mapped element got deleted. In this
+    // case we only normalize the elements (aka. migrate to SLOW_SLOPPY).
+    if (*entry == kMaxUInt32) return dictionary;
+    uint32_t length = elements->parameter_map_length();
+    if (*entry >= length) {
+      *entry = dictionary->FindEntry(*entry - length) + length;
+    }
+    return dictionary;
+  }
+
+  static void SloppyDeleteImpl(Handle<JSObject> obj,
+                               Handle<SloppyArgumentsElements> elements,
+                               uint32_t entry) {
+    // Always normalize element on deleting an entry.
+    NormalizeArgumentsElements(obj, elements, &entry);
+    SlowSloppyArgumentsElementsAccessor::SloppyDeleteImpl(obj, elements, entry);
   }
 
   static void AddImpl(Handle<JSObject> object, uint32_t index,
@@ -3847,14 +3880,10 @@ class FastSloppyArgumentsElementsAccessor
                               Handle<FixedArrayBase> store, uint32_t entry,
                               Handle<Object> value,
                               PropertyAttributes attributes) {
-    Handle<SeededNumberDictionary> dictionary =
-        JSObject::NormalizeElements(object);
-    SloppyArgumentsElements* elements = SloppyArgumentsElements::cast(*store);
-    elements->set_arguments(*dictionary);
-    uint32_t length = elements->parameter_map_length();
-    if (entry >= length) {
-      entry = dictionary->FindEntry(entry - length) + length;
-    }
+    DCHECK_EQ(object->elements(), *store);
+    Handle<SloppyArgumentsElements> elements(
+        SloppyArgumentsElements::cast(*store));
+    NormalizeArgumentsElements(object, elements, &entry);
     SlowSloppyArgumentsElementsAccessor::ReconfigureImpl(object, store, entry,
                                                          value, attributes);
   }

src/objects-debug.cc

@@ -480,12 +480,12 @@ void JSArgumentsObject::JSArgumentsObjectVerify() {
 
 void JSSloppyArgumentsObject::JSSloppyArgumentsObjectVerify() {
   Isolate* isolate = GetIsolate();
+  if (!map()->is_dictionary_map()) VerifyObjectField(kCalleeOffset);
   if (isolate->IsInAnyContext(map(), Context::SLOPPY_ARGUMENTS_MAP_INDEX) ||
       isolate->IsInAnyContext(map(),
                               Context::SLOW_ALIASED_ARGUMENTS_MAP_INDEX) ||
       isolate->IsInAnyContext(map(),
                               Context::FAST_ALIASED_ARGUMENTS_MAP_INDEX)) {
-    // We can only verify the in-object fields for the original maps.
     VerifyObjectField(kLengthOffset);
     VerifyObjectField(kCalleeOffset);
   }
@@ -504,6 +504,7 @@ void SloppyArgumentsElements::SloppyArgumentsElementsVerify(
   if (get(kArgumentsIndex)->IsUndefined(isolate)) return;
 
   ElementsKind kind = holder->GetElementsKind();
+  bool is_fast = kind == FAST_SLOPPY_ARGUMENTS_ELEMENTS;
   CHECK(IsFixedArray());
   CHECK_GE(length(), 2);
   CHECK_EQ(map(), isolate->heap()->sloppy_arguments_elements_map());
@@ -518,7 +519,7 @@ void SloppyArgumentsElements::SloppyArgumentsElementsVerify(
   CHECK_LE(nofMappedParameters, context_object->length());
   CHECK_LE(nofMappedParameters, arg_elements->length());
   ElementsAccessor* accessor;
-  if (kind == FAST_SLOPPY_ARGUMENTS_ELEMENTS) {
+  if (is_fast) {
     accessor = ElementsAccessor::ForKind(FAST_HOLEY_ELEMENTS);
   } else {
     accessor = ElementsAccessor::ForKind(DICTIONARY_ELEMENTS);
@@ -527,12 +528,18 @@ void SloppyArgumentsElements::SloppyArgumentsElementsVerify(
     // Verify that each context-mapped argument is either the hole or a valid
     // Smi within context length range.
     Object* mapped = get_mapped_entry(i);
-    if (mapped->IsTheHole(isolate)) continue;
+    if (mapped->IsTheHole(isolate)) {
+      // Slow sloppy arguments can be holey.
+      if (!is_fast) continue;
+      // Fast sloppy arguments elements are never holey. Either the element is
+      // context-mapped or present in the arguments elements.
+      CHECK(accessor->HasElement(holder, i, arg_elements));
+      continue;
+    }
     Object* value = context_object->get(Smi::cast(mapped)->value());
     CHECK(value->IsObject());
     // None of the context-mapped entries should exist in the arguments
-    // elements unless they have been deleted and readded, which would leave
-    // the_hole in the parameter map.
+    // elements.
     CHECK(!accessor->HasElement(holder, i, arg_elements));
   }
 }

src/objects.cc

@@ -5831,14 +5831,13 @@ Handle<SeededNumberDictionary> JSObject::NormalizeElements(
     Handle<JSObject> object) {
   DCHECK(!object->HasFixedTypedArrayElements());
   Isolate* isolate = object->GetIsolate();
-  bool is_arguments = object->HasSloppyArgumentsElements();
+  bool is_sloppy_arguments = object->HasSloppyArgumentsElements();
   {
     DisallowHeapAllocation no_gc;
     FixedArrayBase* elements = object->elements();
 
-    if (is_arguments) {
-      FixedArray* parameter_map = FixedArray::cast(elements);
-      elements = FixedArrayBase::cast(parameter_map->get(1));
+    if (is_sloppy_arguments) {
+      elements = SloppyArgumentsElements::cast(elements)->arguments();
     }
 
     if (elements->IsDictionary()) {
@@ -5855,7 +5854,7 @@ Handle<SeededNumberDictionary> JSObject::NormalizeElements(
       object->GetElementsAccessor()->Normalize(object);
 
   // Switch to using the dictionary as the backing storage for elements.
-  ElementsKind target_kind = is_arguments
+  ElementsKind target_kind = is_sloppy_arguments
                                  ? SLOW_SLOPPY_ARGUMENTS_ELEMENTS
                                  : object->HasFastStringWrapperElements()
                                        ? SLOW_STRING_WRAPPER_ELEMENTS
@@ -5864,8 +5863,9 @@ Handle<SeededNumberDictionary> JSObject::NormalizeElements(
   // Set the new map first to satify the elements type assert in set_elements().
   JSObject::MigrateToMap(object, new_map);
 
-  if (is_arguments) {
-    FixedArray::cast(object->elements())->set(1, *dictionary);
+  if (is_sloppy_arguments) {
+    SloppyArgumentsElements::cast(object->elements())
+        ->set_arguments(*dictionary);
   } else {
     object->set_elements(*dictionary);
   }
@@ -15451,6 +15451,8 @@ static bool ShouldConvertToFastElements(JSObject* object,
     Object* length = JSArray::cast(object)->length();
     if (!length->IsSmi()) return false;
     *new_capacity = static_cast<uint32_t>(Smi::cast(length)->value());
+  } else if (object->IsJSSloppyArgumentsObject()) {
+    return false;
   } else {
     *new_capacity = dictionary->max_number_key() + 1;
   }
@@ -15495,7 +15497,7 @@ Maybe<bool> JSObject::AddDataElement(Handle<JSObject> object, uint32_t index,
   FixedArrayBase* elements = object->elements();
   ElementsKind dictionary_kind = DICTIONARY_ELEMENTS;
   if (IsSloppyArgumentsElementsKind(kind)) {
-    elements = FixedArrayBase::cast(FixedArray::cast(elements)->get(1));
+    elements = SloppyArgumentsElements::cast(elements)->arguments();
     dictionary_kind = SLOW_SLOPPY_ARGUMENTS_ELEMENTS;
   } else if (IsStringWrapperElementsKind(kind)) {
     dictionary_kind = SLOW_STRING_WRAPPER_ELEMENTS;
@@ -15508,7 +15510,7 @@ Maybe<bool> JSObject::AddDataElement(Handle<JSObject> object, uint32_t index,
                                        SeededNumberDictionary::cast(elements),
                                        index, &new_capacity)
                ? BestFittingFastElementsKind(*object)
-               : dictionary_kind;  // Overwrite in case of arguments.
+               : dictionary_kind;
   } else if (ShouldConvertToSlowElements(
                  *object, static_cast<uint32_t>(elements->length()), index,
                  &new_capacity)) {
@@ -20504,6 +20506,5 @@ ElementsKind JSArrayIterator::ElementsKindForInstanceType(InstanceType type) {
     return kind;
   }
 }
-
 }  // namespace internal
 }  // namespace v8

src/objects.h

@@ -2717,7 +2717,7 @@ class JSSloppyArgumentsObject: public JSArgumentsObject {
   static const int kCalleeOffset = JSArgumentsObject::kHeaderSize;
   static const int kSize = kCalleeOffset + kPointerSize;
   // Indices of in-object properties.
-  static const int kCalleeIndex = 1;
+  static const int kCalleeIndex = kLengthIndex + 1;
 
   DECL_ACCESSORS(callee, Object)
 

test/mjsunit/arguments.js

@@ -248,30 +248,97 @@ assertEquals(117, arg_set(0xFFFFFFFF));
     return arguments
   };
   var args = f(1, 2);
+  %HeapObjectVerify(args);
   assertEquals(1, args[0]);
   assertEquals(2, args[1]);
   assertEquals(key, args[key]);
   assertEquals(2, args.length);
 
   delete args[0];
+  %HeapObjectVerify(args);
   assertEquals(undefined, args[0]);
   assertEquals(2, args[1]);
   assertEquals(key, args[key]);
   assertEquals(2, args.length);
 
   delete args[1];
+  %HeapObjectVerify(args);
   assertEquals(undefined, args[0]);
   assertEquals(undefined, args[1]);
   assertEquals(key, args[key]);
   assertEquals(2, args.length);
 
   delete args[key];
+  %HeapObjectVerify(args);
   assertEquals(undefined, args[0]);
   assertEquals(undefined, args[1]);
   assertEquals(undefined, args[key]);
   assertEquals(2, args.length);
 })();
 
+(function testDeleteSlowSloppyArguments2() {
+  function f(a) {
+    return arguments
+  };
+  var args = f(1, 2);
+  %HeapObjectVerify(args);
+  assertEquals(1, args[0]);
+  assertEquals(2, args[1]);
+  assertEquals(2, args.length);
+
+  delete args[1];
+  %HeapObjectVerify(args);
+  assertEquals(1, args[0]);
+  assertEquals(undefined, args[1]);
+  assertEquals(undefined, args[2]);
+  assertEquals(2, args.length);
+
+  delete args[0];
+  %HeapObjectVerify(args);
+  assertEquals(undefined, args[0]);
+  assertEquals(undefined, args[1]);
+  assertEquals(undefined, args[2]);
+  assertEquals(2, args.length);
+})();
+
+(function testSloppyArgumentProperties() {
+  function f(a, b) { return arguments }
+  let args = f(1, 2, 3, 4);
+  %HeapObjectVerify(args);
+  assertEquals(4, args.length);
+  args.foo = "foo";
+  %HeapObjectVerify(args);
+  assertEquals("foo", args.foo);
+  assertEquals(4, args.length);
+
+  delete args.foo;
+  %HeapObjectVerify(args);
+  assertEquals(undefined, args.foo);
+  assertEquals(4, args.length);
+})();
+
+
+(function testSloppyArgumentsLengthMapChange() {
+  function f(a) { return arguments };
+  let args1 = f(1);
+  let args2 = f(1,2);
+  assertTrue(%HaveSameMap(args1, args2));
+  args2.length = 12;
+  assertTrue(%HaveSameMap(args1, args2));
+  args2.length = "aa"
+  assertTrue(%HaveSameMap(args1, args2));
+
+  let args3 = f(1);
+  let args4 = f(1,2);
+  // Creating holes causes map transitions.
+  assertTrue(%HaveSameMap(args1, args3));
+  assertTrue(%HaveSameMap(args1, args4));
+  delete args3[0];
+  assertFalse(%HaveSameMap(args1, args3));
+  delete args4[1];
+  assertFalse(%HaveSameMap(args1, args4));
+})();
+
 (function testSloppyArgumentsLengthMapChange() {
   function f(a) { return arguments };
   let args1 = f(1);

test/mjsunit/array-slice.js

@@ -25,7 +25,10 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+// Flags: --allow-natives-syntax
+
 // Check that slicing array of holes keeps it as array of holes
+
 (function() {
   var array = new Array(10);
   for (var i = 0; i < 7; i++) {
@@ -222,7 +225,10 @@
 // Check slicing on arguments object.
 (function() {
   function func(expected, a0, a1, a2) {
-    assertEquals(expected, Array.prototype.slice.call(arguments, 1));
+    let result =  Array.prototype.slice.call(arguments, 1);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
+    assertEquals(expected, result);
   }
 
   func([]);
@@ -240,7 +246,10 @@
     assertEquals(undefined, y);
     y = 239;
     assertEquals(1, arguments.length);  // arguments length is the same.
-    assertEquals([x], Array.prototype.slice.call(arguments, 0));
+    let result = Array.prototype.slice.call(arguments, 0);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
+    assertEquals([x], result);
   }
 
   func('a');
@@ -251,7 +260,10 @@
   function func(x, y) {
     assertEquals(1, arguments.length);
     arguments.length = 7;
-    assertEquals([x,,,,,,,], Array.prototype.slice.call(arguments, 0));
+    let result = Array.prototype.slice.call(arguments, 0);
+    assertEquals([x,,,,,,,], result);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
   }
 
   func('a');
@@ -263,7 +275,10 @@
   function func(x, y) {
     assertEquals(1, arguments.length);
     arguments.length = 'foobar';
-    assertEquals([], Array.prototype.slice.call(arguments, 0));
+    let result = Array.prototype.slice.call(arguments, 0);
+    assertEquals([], result);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
   }
 
   func('a');
@@ -275,7 +290,10 @@
   function func(x, y) {
     assertEquals(1, arguments.length);
     arguments[3] = 239;
-    assertEquals([x], Array.prototype.slice.call(arguments, 0));
+    let result = Array.prototype.slice.call(arguments, 0);
+    assertEquals([x], result);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
   }
 
   func('a');
@@ -286,7 +304,10 @@
   function func(x, y, z) {
     assertEquals(3, arguments.length);
     delete arguments[1];
-    assertEquals([x,,z], Array.prototype.slice.call(arguments, 0));
+    let result = Array.prototype.slice.call(arguments, 0);
+    assertEquals([x,,z], result);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
   }
 
   func('a', 'b', 'c');
@@ -300,6 +321,8 @@
     var result = Array.prototype.slice.call(arguments);
     delete arguments.__proto__[1];
     assertEquals([1,5,3], result);
+    %HeapObjectVerify(result);
+    %HeapObjectVerify(arguments);
   }
   f(1,2,3);
 })();

test/mjsunit/function-arguments-duplicate.js

@@ -27,10 +27,14 @@
 
 // Execises ArgumentsAccessStub::GenerateNewNonStrictSlow.
 
+// Flags: --allow-natives-syntax
+
 function f(a, a) {
   assertEquals(2, a);
   assertEquals(1, arguments[0]);
   assertEquals(2, arguments[1]);
+  assertEquals(2, arguments.length);
+  %HeapObjectVerify(arguments);
 }
 
 f(1, 2);

test/mjsunit/regress/regress-4825.js

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --allow-natives-syntax
+
 function enumerate(o) {
   var keys = [];
   for (var key in o) keys.push(key);
@@ -10,11 +12,13 @@ function enumerate(o) {
 
 (function testSlowSloppyArgumentsElements()  {
   function slowSloppyArguments(a, b, c) {
+    %HeapObjectVerify(arguments);
     arguments[10000] = "last";
     arguments[4000] = "first";
     arguments[6000] = "second";
     arguments[5999] = "x";
     arguments[3999] = "y";
+    %HeapObjectVerify(arguments);
     return arguments;
   }
   assertEquals(["0", "1", "2", "3999", "4000", "5999", "6000", "10000"],
@@ -29,10 +33,12 @@ function enumerate(o) {
     Object.defineProperty(arguments, 10000, {
       enumerable: false, configurable: false, value: "NOPE"
     });
+    %HeapObjectVerify(arguments);
     arguments[4000] = "first";
     arguments[6000] = "second";
     arguments[5999] = "x";
     arguments[3999] = "y";
+    %HeapObjectVerify(arguments);
     return arguments;
   }
 
@@ -43,11 +49,13 @@ function enumerate(o) {
                 enumerate(slowSloppyArguments(1,2,3)));
 })();
 
+
 (function testFastSloppyArgumentsElements()  {
   function fastSloppyArguments(a, b, c) {
     arguments[5] = 1;
     arguments[7] = 0;
     arguments[3] = 2;
+    %HeapObjectVerify(arguments);
     return arguments;
   }
   assertEquals(["0", "1", "2", "3", "5", "7"],
@@ -58,7 +66,11 @@ function enumerate(o) {
 
   function fastSloppyArguments2(a, b, c) {
     delete arguments[0];
+    %DebugPrint(arguments);
+    %HeapObjectVerify(arguments);
     arguments[0] = "test";
+    %DebugPrint(arguments);
+    %HeapObjectVerify(arguments);
     return arguments;
   }
 
@@ -71,8 +83,10 @@ function enumerate(o) {
     Object.defineProperty(arguments, 5, {
       enumerable: false, configurable: false, value: "NOPE"
     });
+    %HeapObjectVerify(arguments);
     arguments[7] = 0;
     arguments[3] = 2;
+    %HeapObjectVerify(arguments);
     return arguments;
   }
   assertEquals(
@@ -83,10 +97,12 @@ function enumerate(o) {
 
   function fastSloppyArguments2(a, b, c) {
     delete arguments[0];
+    %HeapObjectVerify(arguments);
     Object.defineProperty(arguments, 1, {
       enumerable: false, configurable: false, value: "NOPE"
     });
     arguments[0] = "test";
+    %HeapObjectVerify(arguments);
     return arguments;
   }
 

test/mjsunit/regress/regress-645680.js

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-gc
-//
+// Flags: --expose-gc --allow-natives-syntax
+
 function getRandomProperty(v, rand) {
   var properties = Object.getOwnPropertyNames(v);
   if ("constructor" && v.constructor.hasOwnProperty()) {; }
@@ -11,10 +11,12 @@ function getRandomProperty(v, rand) {
   return properties[rand % properties.length];
 }
 
-var __v_18 = (function( b) { return arguments; })("foo", NaN, "bar");
-__v_18.__p_293850326 = "foo";
-__v_18.__defineGetter__(getRandomProperty( 990787501), function() {
+var args = (function( b) { return arguments; })("foo", NaN, "bar");
+args.__p_293850326 = "foo";
+%HeapObjectVerify(args);
+args.__defineGetter__(getRandomProperty( 990787501), function() {
   gc();
-  return __v_18.__p_293850326;
+  return args.__p_293850326;
 });
-Array.prototype.indexOf.call(__v_18)
+%HeapObjectVerify(args);
+Array.prototype.indexOf.call(args)