From ae7161e4cb94cce7c087751686c5d7c0691152dc Mon Sep 17 00:00:00 2001 From: "adamk@chromium.org" Date: Wed, 15 Oct 2014 23:53:02 +0000 Subject: [PATCH] Revert "Remove SmartMove, bringing Array methods further into spec compliance" This reverts https://code.google.com/p/v8/source/detail?r=24647 It caused test failures in Array methods in Linux64 OptimizeForSize. BUG=v8:2615 TBR=verwaest@chromium.org LOG=N Review URL: https://codereview.chromium.org/656683003 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24648 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/array.js | 75 ++++++++++-- .../mjsunit/array-functions-prototype-misc.js | 4 - test/mjsunit/array-natives-elements.js | 3 +- test/mjsunit/array-shift2.js | 6 +- test/mjsunit/array-unshift.js | 2 - test/mjsunit/bugs/bug-2615.js | 84 +++++++++++++ test/mjsunit/regress/regress-2615.js | 112 ------------------ test/mjsunit/regress/regress-crbug-412319.js | 2 +- 8 files changed, 158 insertions(+), 130 deletions(-) delete mode 100644 test/mjsunit/regress/regress-2615.js diff --git a/src/array.js b/src/array.js index 8579450ab4..b6f3a89d69 100644 --- a/src/array.js +++ b/src/array.js @@ -231,6 +231,50 @@ function SmartSlice(array, start_i, del_count, len, deleted_elements) { } +// This function implements the optimized splice implementation that can use +// special array operations to handle sparse arrays in a sensible fashion. +function SmartMove(array, start_i, del_count, len, num_additional_args) { + // Move data to new array. + var new_array = new InternalArray(len - del_count + num_additional_args); + var indices = %GetArrayKeys(array, len); + if (IS_NUMBER(indices)) { + var limit = indices; + for (var i = 0; i < start_i && i < limit; ++i) { + var current = array[i]; + if (!IS_UNDEFINED(current) || i in array) { + new_array[i] = current; + } + } + for (var i = start_i + del_count; i < limit; ++i) { + var current = array[i]; + if (!IS_UNDEFINED(current) || i in array) { + new_array[i - del_count + num_additional_args] = current; + } + } + } else { + var length = indices.length; + for (var k = 0; k < length; ++k) { + var key = indices[k]; + if (!IS_UNDEFINED(key)) { + if (key < start_i) { + var current = array[key]; + if (!IS_UNDEFINED(current) || key in array) { + new_array[key] = current; + } + } else if (key >= start_i + del_count) { + var current = array[key]; + if (!IS_UNDEFINED(current) || key in array) { + new_array[key - del_count + num_additional_args] = current; + } + } + } + } + } + // Move contents of new_array into this array + %MoveArrayContents(new_array, array); +} + + // This is part of the old simple-minded splice. We are using it either // because the receiver is not an array (so we have no choice) or because we // know we are not deleting or moving a lot of elements. @@ -248,9 +292,8 @@ function SimpleSlice(array, start_i, del_count, len, deleted_elements) { } -function SimpleMove(array, start_i, del_count, len, num_additional_args, - force) { - if (num_additional_args !== del_count || force) { +function SimpleMove(array, start_i, del_count, len, num_additional_args) { + if (num_additional_args !== del_count) { // Move the existing elements after the elements to be deleted // to the right position in the resulting array. if (num_additional_args > del_count) { @@ -561,7 +604,11 @@ function ArrayShift() { var first = array[0]; - SimpleMove(array, 0, 1, len, 0); + if (IS_ARRAY(array)) { + SmartMove(array, 0, 1, len, 0); + } else { + SimpleMove(array, 0, 1, len, 0); + } array.length = len - 1; @@ -597,7 +644,14 @@ function ArrayUnshift(arg1) { // length == 1 var array = TO_OBJECT_INLINE(this); var len = TO_UINT32(array.length); var num_arguments = %_ArgumentsLength(); - SimpleMove(array, 0, 0, len, num_arguments, true); + var is_sealed = ObjectIsSealed(array); + + if (IS_ARRAY(array) && !is_sealed && len > 0) { + SmartMove(array, 0, 0, len, num_arguments); + } else { + SimpleMove(array, 0, 0, len, num_arguments); + } + for (var i = 0; i < num_arguments; i++) { array[i] = %_Arguments(i); } @@ -752,8 +806,15 @@ function ArraySplice(start, delete_count) { // point, then include those in the estimate of changed elements. changed_elements += len - start_i - del_count; } - SimpleSlice(array, start_i, del_count, len, deleted_elements); - SimpleMove(array, start_i, del_count, len, num_elements_to_add); + if (UseSparseVariant(array, len, IS_ARRAY(array), changed_elements)) { + %NormalizeElements(array); + %NormalizeElements(deleted_elements); + SmartSlice(array, start_i, del_count, len, deleted_elements); + SmartMove(array, start_i, del_count, len, num_elements_to_add); + } else { + SimpleSlice(array, start_i, del_count, len, deleted_elements); + SimpleMove(array, start_i, del_count, len, num_elements_to_add); + } // Insert the arguments into the resulting array in // place of the deleted elements. diff --git a/test/mjsunit/array-functions-prototype-misc.js b/test/mjsunit/array-functions-prototype-misc.js index 2a16d20cd2..74dc9a6be0 100644 --- a/test/mjsunit/array-functions-prototype-misc.js +++ b/test/mjsunit/array-functions-prototype-misc.js @@ -31,8 +31,6 @@ * should work on other objects too, so we test that too. */ -/* - var LARGE = 4000000; var VERYLARGE = 4000000000; @@ -314,5 +312,3 @@ Array.prototype[1] = undefined; // Test http://code.google.com/p/chromium/issues/detail?id=21860 Array.prototype.push.apply([], [1].splice(0, -(-1 % 5))); - -*/ diff --git a/test/mjsunit/array-natives-elements.js b/test/mjsunit/array-natives-elements.js index e567e7f480..d63346d0a4 100644 --- a/test/mjsunit/array-natives-elements.js +++ b/test/mjsunit/array-natives-elements.js @@ -280,7 +280,8 @@ function array_natives_test() { assertEquals([1.1,1,2,3], a4); a4 = [1.1,2,3]; a4.unshift(1); - assertTrue(%HasFastDoubleElements(a4)); + // assertTrue(%HasFastDoubleElements(a4)); + assertTrue(%HasFastObjectElements(a4)); assertEquals([1,1.1,2,3], a4); a4 = [{},2,3]; a4.unshift(1); diff --git a/test/mjsunit/array-shift2.js b/test/mjsunit/array-shift2.js index aa7eb7476c..73d8cd4ff1 100644 --- a/test/mjsunit/array-shift2.js +++ b/test/mjsunit/array-shift2.js @@ -12,7 +12,7 @@ function test(array) { array.shift(); return array; } -assertEquals(["element 1","element 1"], test(["0",,2])); -assertEquals(["element 1","element 1"], test([{},,{}])); +assertEquals(["element 1",2], test(["0",,2])); +assertEquals(["element 1",{}], test([{},,{}])); %OptimizeFunctionOnNextCall(test); -assertEquals(["element 1","element 1"], test([{},,0])); +assertEquals(["element 1",0], test([{},,0])); diff --git a/test/mjsunit/array-unshift.js b/test/mjsunit/array-unshift.js index da2e756809..0eb299a0ce 100644 --- a/test/mjsunit/array-unshift.js +++ b/test/mjsunit/array-unshift.js @@ -190,7 +190,6 @@ assertFalse(array.hasOwnProperty(7)); })(); -/* // Check the behaviour when approaching maximal values for length. (function() { for (var i = 0; i < 7; i++) { @@ -206,7 +205,6 @@ assertEquals(bigNum + 7, new Array(bigNum).unshift(1, 2, 3, 4, 5, 6, 7)); } })(); -*/ (function() { for (var i = 0; i < 7; i++) { diff --git a/test/mjsunit/bugs/bug-2615.js b/test/mjsunit/bugs/bug-2615.js index 3dc7f1df66..51aeaf4924 100644 --- a/test/mjsunit/bugs/bug-2615.js +++ b/test/mjsunit/bugs/bug-2615.js @@ -25,6 +25,90 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +var a = []; +a[0xfffffffe] = 10; +assertThrows("a.unshift(1);", RangeError); +assertEquals(0xffffffff, a.length); +assertEquals(10, a[0xffffffff]); +assertEquals(undefined, a[0xfffffffe]); + +a = [1,2,3]; +a[0xfffffffe] = 10; +assertThrows("a.splice(1,1,7,7,7,7,7);", RangeError); +assertEquals([1,7,7,7,7,7,3], a.slice(0, 7)); +assertEquals(0xffffffff, a.length); +assertEquals(10, a[0xfffffffe + 5 - 1]); + +a = [1]; +Object.defineProperty(a, "1", {writable:false, configurable:false, value: 100}); +assertThrows("a.unshift(4);", TypeError); +assertEquals([1, 100, 100], a); +var desc = Object.getOwnPropertyDescriptor(a, "1"); +assertEquals(false, desc.writable); +assertEquals(false, desc.configurable); + +a = [1]; +var g = function() { return 100; }; +Object.defineProperty(a, "1", {get:g}); +assertThrows("a.unshift(0);", TypeError); +assertEquals([1, 100, 100], a); +desc = Object.getOwnPropertyDescriptor(a, "1"); +assertEquals(false, desc.configurable); +assertEquals(g, desc.get); + +a = [1]; +var c = 0; +var s = function(v) { c += 1; }; +Object.defineProperty(a, "1", {set:s}); +a.unshift(10); +assertEquals([10, undefined, undefined], a); +assertEquals(1, c); +desc = Object.getOwnPropertyDescriptor(a, "1"); +assertEquals(false, desc.configurable); +assertEquals(s, desc.set); + +a = [1]; +Object.defineProperty(a, "1", {configurable:false, value:10}); +assertThrows("a.splice(1,1);", TypeError); +assertEquals([1, 10], a); +desc = Object.getOwnPropertyDescriptor(a, "1"); +assertEquals(false, desc.configurable); + +a = [0,1,2,3,4,5,6]; +Object.defineProperty(a, "3", {configurable:false, writable:false, value:3}); +assertThrows("a.splice(1,4);", TypeError); +assertEquals([0,5,6,3,,,,,], a); +desc = Object.getOwnPropertyDescriptor(a, "3"); +assertEquals(false, desc.configurable); +assertEquals(false, desc.writable); + +a = [0,1,2,3,4,5,6]; +Object.defineProperty(a, "5", {configurable:false, value:5}); +assertThrows("a.splice(1,4);", TypeError); +assertEquals([0,5,6,3,4,5,,,], a); +desc = Object.getOwnPropertyDescriptor(a, "5"); +assertEquals(false, desc.configurable); + +a = [1,2,3,,5]; +Object.defineProperty(a, "1", {configurable:false, writable:true, value:2}); +assertEquals(1, a.shift()); +assertEquals([2,3,,5], a); +desc = Object.getOwnPropertyDescriptor(a, "1"); +assertEquals(false, desc.configurable); +assertEquals(true, desc.writable); +assertThrows("a.shift();", TypeError); +assertEquals([3,3,,5], a); +desc = Object.getOwnPropertyDescriptor(a, "1"); +assertEquals(false, desc.configurable); +assertEquals(true, desc.writable); + +a = [1,2,3]; +Object.defineProperty(a, "2", {configurable:false, value:3}); +assertThrows("a.pop();", TypeError); +assertEquals([1,2,3], a); +desc = Object.getOwnPropertyDescriptor(a, "2"); +assertEquals(false, desc.configurable); + a = [1,2,,,5]; Object.defineProperty(a, "4", {writable:true, configurable:false, value:5}); assertThrows("a.sort();", TypeError); diff --git a/test/mjsunit/regress/regress-2615.js b/test/mjsunit/regress/regress-2615.js deleted file mode 100644 index 184a6098d9..0000000000 --- a/test/mjsunit/regress/regress-2615.js +++ /dev/null @@ -1,112 +0,0 @@ -// Copyright 2013 the V8 project authors. All rights reserved. -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following -// disclaimer in the documentation and/or other materials provided -// with the distribution. -// * Neither the name of Google Inc. nor the names of its -// contributors may be used to endorse or promote products derived -// from this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -/* These tests should pass now but they take too long to run. -var a = []; -a[0xfffffffe] = 10; -assertThrows("a.unshift(1);", RangeError); -assertEquals(0xffffffff, a.length); -assertEquals(10, a[0xffffffff]); -assertEquals(undefined, a[0xfffffffe]); - -a = [1,2,3]; -a[0xfffffffe] = 10; -assertThrows("a.splice(1,1,7,7,7,7,7);", RangeError); -assertEquals([1,7,7,7,7,7,3], a.slice(0, 7)); -assertEquals(0xffffffff, a.length); -assertEquals(10, a[0xfffffffe + 5 - 1]); -*/ - -a = [1]; -Object.defineProperty(a, "1", {writable:false, configurable:false, value: 100}); -assertThrows("a.unshift(4);", TypeError); -assertEquals([1, 100, 100], a); -var desc = Object.getOwnPropertyDescriptor(a, "1"); -assertEquals(false, desc.writable); -assertEquals(false, desc.configurable); - -a = [1]; -var g = function() { return 100; }; -Object.defineProperty(a, "1", {get:g}); -assertThrows("a.unshift(0);", TypeError); -assertEquals([1, 100, 100], a); -desc = Object.getOwnPropertyDescriptor(a, "1"); -assertEquals(false, desc.configurable); -assertEquals(g, desc.get); - -a = [1]; -var c = 0; -var s = function(v) { c += 1; }; -Object.defineProperty(a, "1", {set:s}); -a.unshift(10); -assertEquals([10, undefined, undefined], a); -assertEquals(1, c); -desc = Object.getOwnPropertyDescriptor(a, "1"); -assertEquals(false, desc.configurable); -assertEquals(s, desc.set); - -a = [1]; -Object.defineProperty(a, "1", {configurable:false, value:10}); -assertThrows("a.splice(1,1);", TypeError); -assertEquals([1, 10], a); -desc = Object.getOwnPropertyDescriptor(a, "1"); -assertEquals(false, desc.configurable); - -a = [0,1,2,3,4,5,6]; -Object.defineProperty(a, "3", {configurable:false, writable:false, value:3}); -assertThrows("a.splice(1,4);", TypeError); -assertEquals([0,5,6,3,,,,], a); -desc = Object.getOwnPropertyDescriptor(a, "3"); -assertEquals(false, desc.configurable); -assertEquals(false, desc.writable); - -a = [0,1,2,3,4,5,6]; -Object.defineProperty(a, "5", {configurable:false, value:5}); -assertThrows("a.splice(1,4);", TypeError); -assertEquals([0,5,6,3,4,5,,], a); -desc = Object.getOwnPropertyDescriptor(a, "5"); -assertEquals(false, desc.configurable); - -a = [1,2,3,,5]; -Object.defineProperty(a, "1", {configurable:false, writable:true, value:2}); -assertEquals(1, a.shift()); -assertEquals([2,3,,5], a); -desc = Object.getOwnPropertyDescriptor(a, "1"); -assertEquals(false, desc.configurable); -assertEquals(true, desc.writable); -assertThrows("a.shift();", TypeError); -assertEquals([3,3,,5], a); -desc = Object.getOwnPropertyDescriptor(a, "1"); -assertEquals(false, desc.configurable); -assertEquals(true, desc.writable); - -a = [1,2,3]; -Object.defineProperty(a, "2", {configurable:false, value:3}); -assertThrows("a.pop();", TypeError); -assertEquals([1,2,3], a); -desc = Object.getOwnPropertyDescriptor(a, "2"); -assertEquals(false, desc.configurable); diff --git a/test/mjsunit/regress/regress-crbug-412319.js b/test/mjsunit/regress/regress-crbug-412319.js index c597b0dd87..21386e3bd6 100644 --- a/test/mjsunit/regress/regress-crbug-412319.js +++ b/test/mjsunit/regress/regress-crbug-412319.js @@ -15,5 +15,5 @@ __f_6(); %OptimizeFunctionOnNextCall(__f_6); __f_6(); function __f_7(__v_7) { - __v_7.pop(); + __v_7.push(Infinity); }