Reviewers: adamk,
Message:
I couldn't figure out how to trigger sparse reverse. If anyone has advice,
that'd be appreciated.
Description:
Array.prototype.reverse should call [[HasProperty]] on elements before
[[Get]]
This is a change from ES5 to ES6: When reversing an array, first it is
checked
whether the element exists, before the element is looked up. The order in
ES6
is
[[HasElement]] lower
[[Get]] lower (if present)
[[HasElement]] upper
[[Get]] upper (if present)
In ES5, on the other hand, the order was
[[Get]] lower
[[Get]] upper
[[HasElement]] lower
[[HasElement]] upper
Unfortunately, this patch probably makes reverse a bit slower because the
previous code was able to skip the [[HasElement]] check when the element was
not undefined. (This optimization is invalid with the addition of Proxies,
but those aren't turned on yet.) I have not benchmarked it yet, so I don't
know how big the effect is. It will be a bit of work to recover the
optimization, since any old array could have a getter which modifies the
array and our JavaScript code doesn't tend to look deeply into object maps.
BUG=v8:4223
R=adamk
LOG=Y
Please review this at https://codereview.chromium.org/1238593003/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+24, -12 lines):
M src/array.js
A test/mjsunit/es6/array-reverse-order.js
Index: src/array.js
diff --git a/src/array.js b/src/array.js
index
7baabf83610c3c333eb13b9a5463c652ccaa3839..deec030b5a298aef8a54b2df0b78734abc230817
100644
--- a/src/array.js
+++ b/src/array.js
@@ -567,10 +567,10 @@ function SparseReverse(array, len) {
high = len - i - 1;
}
- var current_i = array[low];
- if (!IS_UNDEFINED(current_i) || low in array) {
- var current_j = array[high];
- if (!IS_UNDEFINED(current_j) || high in array) {
+ if (low in array) {
+ var current_i = array[low];
+ if (high in array) {
+ var current_j = array[high];
array[low] = current_j;
array[high] = current_i;
} else {
@@ -578,8 +578,8 @@ function SparseReverse(array, len) {
delete array[low];
}
} else {
- var current_j = array[high];
- if (!IS_UNDEFINED(current_j) || high in array) {
+ if (high in array) {
+ var current_j = array[high];
array[low] = current_j;
delete array[high];
}
@@ -591,10 +591,10 @@ function SparseReverse(array, len) {
function InnerArrayReverse(array, len) {
var j = len - 1;
for (var i = 0; i < j; i++, j--) {
- var current_i = array[i];
- if (!IS_UNDEFINED(current_i) || i in array) {
- var current_j = array[j];
- if (!IS_UNDEFINED(current_j) || j in array) {
+ if (i in array) {
+ var current_i = array[i];
+ if (j in array) {
+ var current_j = array[j];
array[i] = current_j;
array[j] = current_i;
} else {
@@ -602,8 +602,8 @@ function InnerArrayReverse(array, len) {
delete array[i];
}
} else {
- var current_j = array[j];
- if (!IS_UNDEFINED(current_j) || j in array) {
+ if (j in array) {
+ var current_j = array[j];
array[i] = current_j;
delete array[j];
}
Index: test/mjsunit/es6/array-reverse-order.js
diff --git a/test/mjsunit/es6/array-reverse-order.js
b/test/mjsunit/es6/array-reverse-order.js
new file mode 100644
index
0000000000000000000000000000000000000000..591c36fe908991d49da5b88260010d831fafa1a5
--- /dev/null
+++ b/test/mjsunit/es6/array-reverse-order.js
@@ -0,0 +1,12 @@
+// Copyright 2015 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.
+
+// ES6 specifically says that elements should be checked with
[[HasElement]] before
+// [[Get]]. This is observable in case a getter deletes elements. ES5 put
the
+// [[HasElement]] after the [[Get]].
+// TODO(littledan): Add a test which triggers the sparse codepath. Below,
the
+// non-sparse codepath is tested.
+
+assertTrue(1 in Array.prototype.reverse.call(
+ {length:2, get 0(){delete this[0];}, 1: "b"}))
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
