From ae1fa3daad167fcdc0905574d61df4ecd7e231a3 Mon Sep 17 00:00:00 2001 From: Michael Achenbach Date: Tue, 25 Apr 2017 14:43:05 +0000 Subject: [PATCH] Revert "[runtime] Keep FAST_SLOPPY_ARGUMENTS packed" This reverts commit 28930128ce24c899676b0e15e85095574f7e04c3. Reason for revert: GC stress failures: https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds/12958 Original change's description: > [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: I2afa4fb5f0af9942eee0a1606942f5f289539330 > Reviewed-on: https://chromium-review.googlesource.com/480379 > Commit-Queue: Camillo Bruni > Reviewed-by: Jakob Kummerow > Cr-Commit-Position: refs/heads/master@{#44857} TBR=jkummerow@chromium.org,cbruni@chromium.org,v8-reviews@googlegroups.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: I9482bf693a745d1301d068869ddae39f11143827 Reviewed-on: https://chromium-review.googlesource.com/486885 Reviewed-by: Michael Achenbach Commit-Queue: Michael Achenbach Cr-Commit-Position: refs/heads/master@{#44863} --- src/elements.cc | 72 +++++++------------- src/objects-debug.cc | 17 ++--- src/objects.cc | 21 +++--- src/objects.h | 2 +- test/mjsunit/arguments.js | 67 ------------------ test/mjsunit/array-slice.js | 30 ++------ test/mjsunit/function-arguments-duplicate.js | 4 -- test/mjsunit/regress/regress-4825.js | 16 ----- test/mjsunit/regress/regress-645680.js | 16 ++--- 9 files changed, 53 insertions(+), 192 deletions(-) diff --git a/src/elements.cc b/src/elements.cc index ac078a478a..a23e6ff2d9 100644 --- a/src/elements.cc +++ b/src/elements.cc @@ -3439,8 +3439,6 @@ 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; } @@ -3464,21 +3462,17 @@ class SloppyArgumentsElementsAccessor } static void DeleteImpl(Handle obj, uint32_t entry) { - Handle elements( - SloppyArgumentsElements::cast(obj->elements())); + SloppyArgumentsElements* elements = + SloppyArgumentsElements::cast(obj->elements()); uint32_t length = elements->parameter_map_length(); 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()); - entry = kMaxUInt32; + } else { + Subclass::DeleteFromArguments(obj, entry - length); } - Subclass::SloppyDeleteImpl(obj, elements, entry); - } - - static void SloppyDeleteImpl(Handle obj, - Handle elements, - uint32_t entry) { - // Implemented in subclasses. - UNREACHABLE(); } static void CollectElementIndicesImpl(Handle object, @@ -3631,21 +3625,17 @@ class SlowSloppyArgumentsElementsAccessor } return result; } - static void SloppyDeleteImpl(Handle obj, - Handle elements, - uint32_t entry) { - // No need to delete a context mapped entry from the arguments elements. - if (entry == kMaxUInt32) return; + static void DeleteFromArguments(Handle obj, uint32_t entry) { Isolate* isolate = obj->GetIsolate(); + Handle elements( + SloppyArgumentsElements::cast(obj->elements()), isolate); Handle dict( SeededNumberDictionary::cast(elements->arguments()), isolate); // TODO(verwaest): Remove reliance on index in Shrink. uint32_t index = GetIndexForEntryImpl(*dict, entry); - int length = elements->parameter_map_length(); - Handle result = - SeededNumberDictionary::DeleteProperty(dict, entry - length); + Handle result = SeededNumberDictionary::DeleteProperty(dict, entry); USE(result); - DCHECK(result->IsTrue(isolate)); + DCHECK(result->IsTrue(dict->GetIsolate())); Handle new_elements = SeededNumberDictionary::Shrink(dict, index); elements->set_arguments(*new_elements); @@ -3769,28 +3759,10 @@ class FastSloppyArgumentsElementsAccessor return FastHoleyObjectElementsAccessor::NormalizeImpl(object, arguments); } - static Handle NormalizeArgumentsElements( - Handle object, Handle elements, - uint32_t* entry) { - Handle 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 obj, - Handle elements, - uint32_t entry) { - // Always normalize element on deleting an entry. - NormalizeArgumentsElements(obj, elements, &entry); - SlowSloppyArgumentsElementsAccessor::SloppyDeleteImpl(obj, elements, entry); + static void DeleteFromArguments(Handle obj, uint32_t entry) { + Handle arguments = + GetArguments(obj->GetIsolate(), obj->elements()); + FastHoleyObjectElementsAccessor::DeleteCommon(obj, entry, arguments); } static void AddImpl(Handle object, uint32_t index, @@ -3818,10 +3790,14 @@ class FastSloppyArgumentsElementsAccessor Handle store, uint32_t entry, Handle value, PropertyAttributes attributes) { - DCHECK_EQ(object->elements(), *store); - Handle elements( - SloppyArgumentsElements::cast(*store)); - NormalizeArgumentsElements(object, elements, &entry); + Handle 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; + } SlowSloppyArgumentsElementsAccessor::ReconfigureImpl(object, store, entry, value, attributes); } diff --git a/src/objects-debug.cc b/src/objects-debug.cc index 490b4124af..803e41c54f 100644 --- a/src/objects-debug.cc +++ b/src/objects-debug.cc @@ -476,12 +476,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); } @@ -500,7 +500,6 @@ 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()); @@ -515,7 +514,7 @@ void SloppyArgumentsElements::SloppyArgumentsElementsVerify( CHECK_LE(nofMappedParameters, context_object->length()); CHECK_LE(nofMappedParameters, arg_elements->length()); ElementsAccessor* accessor; - if (is_fast) { + if (kind == FAST_SLOPPY_ARGUMENTS_ELEMENTS) { accessor = ElementsAccessor::ForKind(FAST_HOLEY_ELEMENTS); } else { accessor = ElementsAccessor::ForKind(DICTIONARY_ELEMENTS); @@ -524,18 +523,12 @@ 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)) { - // 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; - } + if (mapped->IsTheHole(isolate)) 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. + // elements unless they have been deleted and readded, which would leave + // the_hole in the parameter map. CHECK(!accessor->HasElement(holder, i, arg_elements)); } } diff --git a/src/objects.cc b/src/objects.cc index 503f580abc..1ff56694c0 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -5826,13 +5826,14 @@ Handle JSObject::NormalizeElements( Handle object) { DCHECK(!object->HasFixedTypedArrayElements()); Isolate* isolate = object->GetIsolate(); - bool is_sloppy_arguments = object->HasSloppyArgumentsElements(); + bool is_arguments = object->HasSloppyArgumentsElements(); { DisallowHeapAllocation no_gc; FixedArrayBase* elements = object->elements(); - if (is_sloppy_arguments) { - elements = SloppyArgumentsElements::cast(elements)->arguments(); + if (is_arguments) { + FixedArray* parameter_map = FixedArray::cast(elements); + elements = FixedArrayBase::cast(parameter_map->get(1)); } if (elements->IsDictionary()) { @@ -5849,7 +5850,7 @@ Handle JSObject::NormalizeElements( object->GetElementsAccessor()->Normalize(object); // Switch to using the dictionary as the backing storage for elements. - ElementsKind target_kind = is_sloppy_arguments + ElementsKind target_kind = is_arguments ? SLOW_SLOPPY_ARGUMENTS_ELEMENTS : object->HasFastStringWrapperElements() ? SLOW_STRING_WRAPPER_ELEMENTS @@ -5858,9 +5859,8 @@ Handle JSObject::NormalizeElements( // Set the new map first to satify the elements type assert in set_elements(). JSObject::MigrateToMap(object, new_map); - if (is_sloppy_arguments) { - SloppyArgumentsElements::cast(object->elements()) - ->set_arguments(*dictionary); + if (is_arguments) { + FixedArray::cast(object->elements())->set(1, *dictionary); } else { object->set_elements(*dictionary); } @@ -15597,8 +15597,6 @@ static bool ShouldConvertToFastElements(JSObject* object, Object* length = JSArray::cast(object)->length(); if (!length->IsSmi()) return false; *new_capacity = static_cast(Smi::cast(length)->value()); - } else if (object->IsJSSloppyArgumentsObject()) { - return false; } else { *new_capacity = dictionary->max_number_key() + 1; } @@ -15643,7 +15641,7 @@ Maybe JSObject::AddDataElement(Handle object, uint32_t index, FixedArrayBase* elements = object->elements(); ElementsKind dictionary_kind = DICTIONARY_ELEMENTS; if (IsSloppyArgumentsElementsKind(kind)) { - elements = SloppyArgumentsElements::cast(elements)->arguments(); + elements = FixedArrayBase::cast(FixedArray::cast(elements)->get(1)); dictionary_kind = SLOW_SLOPPY_ARGUMENTS_ELEMENTS; } else if (IsStringWrapperElementsKind(kind)) { dictionary_kind = SLOW_STRING_WRAPPER_ELEMENTS; @@ -15656,7 +15654,7 @@ Maybe JSObject::AddDataElement(Handle object, uint32_t index, SeededNumberDictionary::cast(elements), index, &new_capacity) ? BestFittingFastElementsKind(*object) - : dictionary_kind; + : dictionary_kind; // Overwrite in case of arguments. } else if (ShouldConvertToSlowElements( *object, static_cast(elements->length()), index, &new_capacity)) { @@ -20645,5 +20643,6 @@ ElementsKind JSArrayIterator::ElementsKindForInstanceType(InstanceType type) { return kind; } } + } // namespace internal } // namespace v8 diff --git a/src/objects.h b/src/objects.h index 234ce66331..00144d2ae3 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2707,7 +2707,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 = kLengthIndex + 1; + static const int kCalleeIndex = 1; DECL_ACCESSORS(callee, Object) diff --git a/test/mjsunit/arguments.js b/test/mjsunit/arguments.js index 09a506b4a4..97ec7cca6d 100644 --- a/test/mjsunit/arguments.js +++ b/test/mjsunit/arguments.js @@ -248,93 +248,26 @@ 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)); -})(); diff --git a/test/mjsunit/array-slice.js b/test/mjsunit/array-slice.js index a6854c0b26..b017dd506a 100644 --- a/test/mjsunit/array-slice.js +++ b/test/mjsunit/array-slice.js @@ -25,10 +25,7 @@ // (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++) { @@ -225,9 +222,7 @@ // Check slicing on arguments object. (function() { function func(expected, a0, a1, a2) { - let result = Array.prototype.slice.call(arguments, 1); - assertEquals(expected, result); - %HeapObjectVerify(result); + assertEquals(expected, Array.prototype.slice.call(arguments, 1)); } func([]); @@ -245,9 +240,7 @@ assertEquals(undefined, y); y = 239; assertEquals(1, arguments.length); // arguments length is the same. - let result = Array.prototype.slice.call(arguments, 0); - assertEquals([x], result); - %HeapObjectVerify(result); + assertEquals([x], Array.prototype.slice.call(arguments, 0)); } func('a'); @@ -258,10 +251,7 @@ function func(x, y) { assertEquals(1, arguments.length); arguments.length = 7; - let result = Array.prototype.slice.call(arguments, 0); - assertEquals([x,,,,,,,], result); - %HeapObjectVerify(result); - %HeapObjectVerify(arguments); + assertEquals([x,,,,,,,], Array.prototype.slice.call(arguments, 0)); } func('a'); @@ -273,10 +263,7 @@ function func(x, y) { assertEquals(1, arguments.length); arguments.length = 'foobar'; - let result = Array.prototype.slice.call(arguments, 0); - assertEquals([], result); - %HeapObjectVerify(result); - %HeapObjectVerify(arguments); + assertEquals([], Array.prototype.slice.call(arguments, 0)); } func('a'); @@ -288,9 +275,7 @@ function func(x, y) { assertEquals(1, arguments.length); arguments[3] = 239; - let result = Array.prototype.slice.call(arguments, 0); - assertEquals([x], result); - %HeapObjectVerify(result); + assertEquals([x], Array.prototype.slice.call(arguments, 0)); } func('a'); @@ -301,9 +286,7 @@ function func(x, y, z) { assertEquals(3, arguments.length); delete arguments[1]; - let result = Array.prototype.slice.call(arguments, 0); - assertEquals([x,,z], result); - %HeapObjectVerify(result); + assertEquals([x,,z], Array.prototype.slice.call(arguments, 0)); } func('a', 'b', 'c'); @@ -317,7 +300,6 @@ var result = Array.prototype.slice.call(arguments); delete arguments.__proto__[1]; assertEquals([1,5,3], result); - %HeapObjectVerify(result); } f(1,2,3); })(); diff --git a/test/mjsunit/function-arguments-duplicate.js b/test/mjsunit/function-arguments-duplicate.js index a0ec37ca10..80f03a106b 100644 --- a/test/mjsunit/function-arguments-duplicate.js +++ b/test/mjsunit/function-arguments-duplicate.js @@ -27,14 +27,10 @@ // 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); diff --git a/test/mjsunit/regress/regress-4825.js b/test/mjsunit/regress/regress-4825.js index fafd3db73b..5ad096f3ed 100644 --- a/test/mjsunit/regress/regress-4825.js +++ b/test/mjsunit/regress/regress-4825.js @@ -2,8 +2,6 @@ // 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); @@ -12,13 +10,11 @@ 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"], @@ -33,12 +29,10 @@ 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; } @@ -49,13 +43,11 @@ 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"], @@ -66,11 +58,7 @@ function enumerate(o) { function fastSloppyArguments2(a, b, c) { delete arguments[0]; - %DebugPrint(arguments); - %HeapObjectVerify(arguments); arguments[0] = "test"; - %DebugPrint(arguments); - %HeapObjectVerify(arguments); return arguments; } @@ -83,10 +71,8 @@ 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( @@ -97,12 +83,10 @@ 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; } diff --git a/test/mjsunit/regress/regress-645680.js b/test/mjsunit/regress/regress-645680.js index de216f07fc..b244d9c047 100644 --- a/test/mjsunit/regress/regress-645680.js +++ b/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 --allow-natives-syntax - +// Flags: --expose-gc +// function getRandomProperty(v, rand) { var properties = Object.getOwnPropertyNames(v); if ("constructor" && v.constructor.hasOwnProperty()) {; } @@ -11,12 +11,10 @@ function getRandomProperty(v, rand) { return properties[rand % properties.length]; } -var args = (function( b) { return arguments; })("foo", NaN, "bar"); -args.__p_293850326 = "foo"; -%HeapObjectVerify(args); -args.__defineGetter__(getRandomProperty( 990787501), function() { +var __v_18 = (function( b) { return arguments; })("foo", NaN, "bar"); +__v_18.__p_293850326 = "foo"; +__v_18.__defineGetter__(getRandomProperty( 990787501), function() { gc(); - return args.__p_293850326; + return __v_18.__p_293850326; }); -%HeapObjectVerify(args); -Array.prototype.indexOf.call(args) +Array.prototype.indexOf.call(__v_18)