Commit 6da8e1f9 authored by Michael Stanton's avatar Michael Stanton Committed by Commit Bot

Revert "[Builtins] Array.prototype.forEach perf regression on dictionaries."

This reverts commit 34625fdb.

Reason for revert: Test caused timeout, investigating.

Original change's description:
> [Builtins] Array.prototype.forEach perf regression on dictionaries.
> 
> An unnecessary call to ToString() on the array index caused trips to
> the runtime. The fix also includes performance micro-benchmarks so
> we'll have a harder time regressing this case in future.
> 
> Bug: v8:8112
> Change-Id: Iada5bd2e3c6d2246fb1225e7094f3d9c66ddafbd
> Reviewed-on: https://chromium-review.googlesource.com/1206355
> Commit-Queue: Michael Stanton <mvstanton@chromium.org>
> Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#55653}

TBR=mvstanton@chromium.org,tebbi@chromium.org

Change-Id: I21de9b9b33edf03f2173f579c4ba0fc3dfd8ff88
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:8112
Reviewed-on: https://chromium-review.googlesource.com/1209288Reviewed-by: 's avatarMichael Stanton <mvstanton@chromium.org>
Commit-Queue: Michael Stanton <mvstanton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55681}
parent ea5ffdfd
...@@ -10,16 +10,15 @@ module array { ...@@ -10,16 +10,15 @@ module array {
// 6. Repeat, while k < len // 6. Repeat, while k < len
for (let k: Smi = initial_k; k < len; k = k + 1) { for (let k: Smi = initial_k; k < len; k = k + 1) {
// 6a. Let Pk be ! ToString(k). // 6a. Let Pk be ! ToString(k).
// k is guaranteed to be a positive integer, hence ToString is const pK: String = ToString_Inline(context, k);
// side-effect free and HasProperty/GetProperty do the conversion inline.
// 6b. Let kPresent be ? HasProperty(O, Pk). // 6b. Let kPresent be ? HasProperty(O, Pk).
const kPresent: Boolean = HasProperty_Inline(context, o, k); const kPresent: Boolean = HasProperty(context, o, pK);
// 6c. If kPresent is true, then // 6c. If kPresent is true, then
if (kPresent == True) { if (kPresent == True) {
// 6c. i. Let kValue be ? Get(O, Pk). // 6c. i. Let kValue be ? Get(O, Pk).
const kValue: Object = GetProperty(context, o, k); const kValue: Object = GetProperty(context, o, pK);
// 6c. ii. Perform ? Call(callbackfn, T, <kValue, k, O>). // 6c. ii. Perform ? Call(callbackfn, T, <kValue, k, O>).
Call(context, callbackfn, thisArg, kValue, k, o); Call(context, callbackfn, thisArg, kValue, k, o);
......
...@@ -191,7 +191,6 @@ extern macro GetProperty(Context, Object, Object): Object; ...@@ -191,7 +191,6 @@ extern macro GetProperty(Context, Object, Object): Object;
extern builtin SetProperty(Context, Object, Object, Object); extern builtin SetProperty(Context, Object, Object, Object);
extern builtin DeleteProperty(Context, Object, Object, LanguageMode); extern builtin DeleteProperty(Context, Object, Object, LanguageMode);
extern builtin HasProperty(Context, JSReceiver, Object): Boolean; extern builtin HasProperty(Context, JSReceiver, Object): Boolean;
extern macro HasProperty_Inline(Context, JSReceiver, Object): Boolean;
extern macro ThrowRangeError(Context, constexpr MessageTemplate): never; extern macro ThrowRangeError(Context, constexpr MessageTemplate): never;
extern macro ThrowTypeError(Context, constexpr MessageTemplate): never; extern macro ThrowTypeError(Context, constexpr MessageTemplate): never;
......
...@@ -2739,13 +2739,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler { ...@@ -2739,13 +2739,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
SloppyTNode<Object> key, SloppyTNode<Object> key,
HasPropertyLookupMode mode); HasPropertyLookupMode mode);
// Due to naming conflict with the builtin function namespace.
TNode<Oddball> HasProperty_Inline(TNode<Context> context,
TNode<Object> object, TNode<Object> key) {
return HasProperty(context, object, key,
HasPropertyLookupMode::kHasProperty);
}
Node* Typeof(Node* value); Node* Typeof(Node* value);
TNode<Object> GetSuperConstructor(SloppyTNode<Context> context, TNode<Object> GetSuperConstructor(SloppyTNode<Context> context,
......
...@@ -30,7 +30,6 @@ DefineHigherOrderTests([ ...@@ -30,7 +30,6 @@ DefineHigherOrderTests([
['DoubleEvery', newClosure('every'), DoubleSetup, v => v > 0.0], ['DoubleEvery', newClosure('every'), DoubleSetup, v => v > 0.0],
['SmiEvery', newClosure('every'), SmiSetup, v => v != 34343], ['SmiEvery', newClosure('every'), SmiSetup, v => v != 34343],
['FastEvery', newClosure('every'), FastSetup, v => v !== 'hi'], ['FastEvery', newClosure('every'), FastSetup, v => v !== 'hi'],
['DictionaryEvery', newClosure('every'), DictionarySetup, v => v != 34343],
['OptFastEvery', OptFastEvery, FastSetup, v => true], ['OptFastEvery', OptFastEvery, FastSetup, v => true],
['OptUnreliableEvery', OptUnreliableEvery, FastSetup, v => true] ['OptUnreliableEvery', OptUnreliableEvery, FastSetup, v => true]
]); ]);
......
...@@ -61,10 +61,6 @@ DefineHigherOrderTests([ ...@@ -61,10 +61,6 @@ DefineHigherOrderTests([
], ],
['SmiFilter', newClosure('filter'), SmiSetup, v => v % 2 === 0], ['SmiFilter', newClosure('filter'), SmiSetup, v => v % 2 === 0],
['FastFilter', newClosure('filter'), FastSetup, (_, i) => i % 2 === 0], ['FastFilter', newClosure('filter'), FastSetup, (_, i) => i % 2 === 0],
[
'DictionaryFilter', newClosure('filter'), DictionarySetup,
v => true
],
[ [
'GenericFilter', newClosure('filter', true), ObjectSetup, 'GenericFilter', newClosure('filter', true), ObjectSetup,
(_, i) => i % 2 === 0 (_, i) => i % 2 === 0
......
...@@ -60,10 +60,6 @@ DefineHigherOrderTests([ ...@@ -60,10 +60,6 @@ DefineHigherOrderTests([
'FastForEach', newClosure('forEach'), FastSetup, 'FastForEach', newClosure('forEach'), FastSetup,
v => v === `value ${max_index}` v => v === `value ${max_index}`
], ],
[
'DictionaryForEach', newClosure('forEach'), DictionarySetup,
v => v === max_index
],
[ [
'GenericForEach', newClosure('forEach', true), ObjectSetup, 'GenericForEach', newClosure('forEach', true), ObjectSetup,
v => v === max_index v => v === max_index
......
...@@ -55,7 +55,6 @@ DefineHigherOrderTests([ ...@@ -55,7 +55,6 @@ DefineHigherOrderTests([
['FastMap', newClosure('map'), FastSetup, v => v], ['FastMap', newClosure('map'), FastSetup, v => v],
['SmallSmiToDoubleMap', newClosure('map'), SmiSetup, v => v + 0.5], ['SmallSmiToDoubleMap', newClosure('map'), SmiSetup, v => v + 0.5],
['SmallSmiToFastMap', newClosure('map'), SmiSetup, v => 'hi' + v], ['SmallSmiToFastMap', newClosure('map'), SmiSetup, v => 'hi' + v],
['DictionaryMap', newClosure('map'), DictionarySetup, v => v],
['GenericMap', newClosure('map', true), ObjectSetup, v => v], ['GenericMap', newClosure('map', true), ObjectSetup, v => v],
['OptFastMap', OptFastMap, FastSetup, undefined], ['OptFastMap', OptFastMap, FastSetup, undefined],
['OptUnreliableMap', OptUnreliableMap, FastSetup, v => v] ['OptUnreliableMap', OptUnreliableMap, FastSetup, v => v]
......
...@@ -67,12 +67,10 @@ function HoleyFastSetup() { ...@@ -67,12 +67,10 @@ function HoleyFastSetup() {
function DictionarySetup() { function DictionarySetup() {
array = []; array = [];
// Add a large index to force dictionary elements. // Add a large index to force dictionary elements.
array[1000000] = 0; array[2**30] = 10;
var len = array.length; // Spread out {array_size} elements.
for (var i = 0; i < len; i++) { for (var i = 0; i < array_size-1; i++) {
if (i % 100 === 0) { array[i*101] = i;
array[i] = i;
}
} }
assert(%HasDictionaryElements(array)); assert(%HasDictionaryElements(array));
} }
......
...@@ -30,7 +30,6 @@ DefineHigherOrderTests([ ...@@ -30,7 +30,6 @@ DefineHigherOrderTests([
['DoubleSome', newClosure('some'), DoubleSetup, v => v < 0.0], ['DoubleSome', newClosure('some'), DoubleSetup, v => v < 0.0],
['SmiSome', newClosure('some'), SmiSetup, v => v === 34343], ['SmiSome', newClosure('some'), SmiSetup, v => v === 34343],
['FastSome', newClosure('some'), FastSetup, v => v === 'hi'], ['FastSome', newClosure('some'), FastSetup, v => v === 'hi'],
['DictionarySome', newClosure('some'), DictionarySetup, v => v > 0],
['OptFastSome', OptFastSome, FastSetup, undefined], ['OptFastSome', OptFastSome, FastSetup, undefined],
['OptUnreliableSome', OptUnreliableSome, FastSetup, v => v === 'hi'] ['OptUnreliableSome', OptUnreliableSome, FastSetup, v => v === 'hi']
]); ]);
......
...@@ -695,7 +695,6 @@ ...@@ -695,7 +695,6 @@
{"name": "DoubleForEach"}, {"name": "DoubleForEach"},
{"name": "SmiForEach"}, {"name": "SmiForEach"},
{"name": "FastForEach"}, {"name": "FastForEach"},
{"name": "DictionaryForEach"},
{"name": "GenericForEach"}, {"name": "GenericForEach"},
{"name": "OptFastForEach"}, {"name": "OptFastForEach"},
{"name": "OptUnreliableForEach"}, {"name": "OptUnreliableForEach"},
...@@ -703,7 +702,6 @@ ...@@ -703,7 +702,6 @@
{"name": "DoubleFilter"}, {"name": "DoubleFilter"},
{"name": "SmiFilter"}, {"name": "SmiFilter"},
{"name": "FastFilter"}, {"name": "FastFilter"},
{"name": "DictionaryFilter"},
{"name": "GenericFilter"}, {"name": "GenericFilter"},
{"name": "OptFastFilter"}, {"name": "OptFastFilter"},
{"name": "OptUnreliableFilter"}, {"name": "OptUnreliableFilter"},
...@@ -711,14 +709,12 @@ ...@@ -711,14 +709,12 @@
{"name": "DoubleMap"}, {"name": "DoubleMap"},
{"name": "SmiMap"}, {"name": "SmiMap"},
{"name": "FastMap"}, {"name": "FastMap"},
{"name": "DictionaryMap"},
{"name": "GenericMap"}, {"name": "GenericMap"},
{"name": "OptFastMap"}, {"name": "OptFastMap"},
{"name": "OptUnreliableMap"}, {"name": "OptUnreliableMap"},
{"name": "DoubleEvery"}, {"name": "DoubleEvery"},
{"name": "SmiEvery"}, {"name": "SmiEvery"},
{"name": "FastEvery"}, {"name": "FastEvery"},
{"name": "DictionaryEvery"},
{"name": "OptFastEvery"}, {"name": "OptFastEvery"},
{"name": "OptUnreliableEvery"}, {"name": "OptUnreliableEvery"},
{"name": "SmiJoin"}, {"name": "SmiJoin"},
...@@ -728,7 +724,6 @@ ...@@ -728,7 +724,6 @@
{"name": "DoubleSome"}, {"name": "DoubleSome"},
{"name": "SmiSome"}, {"name": "SmiSome"},
{"name": "FastSome"}, {"name": "FastSome"},
{"name": "DictionarySome"},
{"name": "OptFastSome"}, {"name": "OptFastSome"},
{"name": "OptUnreliableSome"}, {"name": "OptUnreliableSome"},
{"name": "DoubleReduce"}, {"name": "DoubleReduce"},
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment