From 34625fdb5afce630c59c34fde723ba7507313d86 Mon Sep 17 00:00:00 2001 From: Mike Stanton Date: Wed, 5 Sep 2018 15:06:48 +0200 Subject: [PATCH] [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 Reviewed-by: Tobias Tebbi Cr-Commit-Position: refs/heads/master@{#55653} --- src/builtins/array-foreach.tq | 7 ++++--- src/builtins/base.tq | 1 + src/code-stub-assembler.h | 7 +++++++ test/js-perf-test/Array/every.js | 1 + test/js-perf-test/Array/filter.js | 4 ++++ test/js-perf-test/Array/for-each.js | 4 ++++ test/js-perf-test/Array/map.js | 1 + test/js-perf-test/Array/run.js | 10 ++++++---- test/js-perf-test/Array/some.js | 1 + test/js-perf-test/JSTests.json | 5 +++++ 10 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/builtins/array-foreach.tq b/src/builtins/array-foreach.tq index c0e19c0803..b934a11b25 100644 --- a/src/builtins/array-foreach.tq +++ b/src/builtins/array-foreach.tq @@ -10,15 +10,16 @@ module array { // 6. Repeat, while k < len for (let k: Smi = initial_k; k < len; k = k + 1) { // 6a. Let Pk be ! ToString(k). - const pK: String = ToString_Inline(context, k); + // k is guaranteed to be a positive integer, hence ToString is + // side-effect free and HasProperty/GetProperty do the conversion inline. // 6b. Let kPresent be ? HasProperty(O, Pk). - const kPresent: Boolean = HasProperty(context, o, pK); + const kPresent: Boolean = HasProperty_Inline(context, o, k); // 6c. If kPresent is true, then if (kPresent == True) { // 6c. i. Let kValue be ? Get(O, Pk). - const kValue: Object = GetProperty(context, o, pK); + const kValue: Object = GetProperty(context, o, k); // 6c. ii. Perform ? Call(callbackfn, T, ). Call(context, callbackfn, thisArg, kValue, k, o); diff --git a/src/builtins/base.tq b/src/builtins/base.tq index be7170421f..208ce7a09d 100644 --- a/src/builtins/base.tq +++ b/src/builtins/base.tq @@ -191,6 +191,7 @@ extern macro GetProperty(Context, Object, Object): Object; extern builtin SetProperty(Context, Object, Object, Object); extern builtin DeleteProperty(Context, Object, Object, LanguageMode); extern builtin HasProperty(Context, JSReceiver, Object): Boolean; +extern macro HasProperty_Inline(Context, JSReceiver, Object): Boolean; extern macro ThrowRangeError(Context, constexpr MessageTemplate): never; extern macro ThrowTypeError(Context, constexpr MessageTemplate): never; diff --git a/src/code-stub-assembler.h b/src/code-stub-assembler.h index 52e8ff040c..7e4f77fd5a 100644 --- a/src/code-stub-assembler.h +++ b/src/code-stub-assembler.h @@ -2738,6 +2738,13 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler { SloppyTNode key, HasPropertyLookupMode mode); + // Due to naming conflict with the builtin function namespace. + TNode HasProperty_Inline(TNode context, + TNode object, TNode key) { + return HasProperty(context, object, key, + HasPropertyLookupMode::kHasProperty); + } + Node* Typeof(Node* value); TNode GetSuperConstructor(SloppyTNode context, diff --git a/test/js-perf-test/Array/every.js b/test/js-perf-test/Array/every.js index 6e9425543a..1c152ba8fb 100644 --- a/test/js-perf-test/Array/every.js +++ b/test/js-perf-test/Array/every.js @@ -30,6 +30,7 @@ DefineHigherOrderTests([ ['DoubleEvery', newClosure('every'), DoubleSetup, v => v > 0.0], ['SmiEvery', newClosure('every'), SmiSetup, v => v != 34343], ['FastEvery', newClosure('every'), FastSetup, v => v !== 'hi'], + ['DictionaryEvery', newClosure('every'), DictionarySetup, v => v != 34343], ['OptFastEvery', OptFastEvery, FastSetup, v => true], ['OptUnreliableEvery', OptUnreliableEvery, FastSetup, v => true] ]); diff --git a/test/js-perf-test/Array/filter.js b/test/js-perf-test/Array/filter.js index 4ceaf5cce2..49c24c73b6 100644 --- a/test/js-perf-test/Array/filter.js +++ b/test/js-perf-test/Array/filter.js @@ -61,6 +61,10 @@ DefineHigherOrderTests([ ], ['SmiFilter', newClosure('filter'), SmiSetup, v => v % 2 === 0], ['FastFilter', newClosure('filter'), FastSetup, (_, i) => i % 2 === 0], + [ + 'DictionaryFilter', newClosure('filter'), DictionarySetup, + v => true + ], [ 'GenericFilter', newClosure('filter', true), ObjectSetup, (_, i) => i % 2 === 0 diff --git a/test/js-perf-test/Array/for-each.js b/test/js-perf-test/Array/for-each.js index c87d5406e0..6d7038c273 100644 --- a/test/js-perf-test/Array/for-each.js +++ b/test/js-perf-test/Array/for-each.js @@ -60,6 +60,10 @@ DefineHigherOrderTests([ 'FastForEach', newClosure('forEach'), FastSetup, v => v === `value ${max_index}` ], + [ + 'DictionaryForEach', newClosure('forEach'), DictionarySetup, + v => v === max_index + ], [ 'GenericForEach', newClosure('forEach', true), ObjectSetup, v => v === max_index diff --git a/test/js-perf-test/Array/map.js b/test/js-perf-test/Array/map.js index 4b278b8882..cadd318993 100644 --- a/test/js-perf-test/Array/map.js +++ b/test/js-perf-test/Array/map.js @@ -55,6 +55,7 @@ DefineHigherOrderTests([ ['FastMap', newClosure('map'), FastSetup, v => v], ['SmallSmiToDoubleMap', newClosure('map'), SmiSetup, v => v + 0.5], ['SmallSmiToFastMap', newClosure('map'), SmiSetup, v => 'hi' + v], + ['DictionaryMap', newClosure('map'), DictionarySetup, v => v], ['GenericMap', newClosure('map', true), ObjectSetup, v => v], ['OptFastMap', OptFastMap, FastSetup, undefined], ['OptUnreliableMap', OptUnreliableMap, FastSetup, v => v] diff --git a/test/js-perf-test/Array/run.js b/test/js-perf-test/Array/run.js index e8b6ef0024..e49dc55ded 100644 --- a/test/js-perf-test/Array/run.js +++ b/test/js-perf-test/Array/run.js @@ -67,10 +67,12 @@ function HoleyFastSetup() { function DictionarySetup() { array = []; // Add a large index to force dictionary elements. - array[2**30] = 10; - // Spread out {array_size} elements. - for (var i = 0; i < array_size-1; i++) { - array[i*101] = i; + array[1000000] = 0; + var len = array.length; + for (var i = 0; i < len; i++) { + if (i % 100 === 0) { + array[i] = i; + } } assert(%HasDictionaryElements(array)); } diff --git a/test/js-perf-test/Array/some.js b/test/js-perf-test/Array/some.js index d7d5efa908..2fc58af849 100644 --- a/test/js-perf-test/Array/some.js +++ b/test/js-perf-test/Array/some.js @@ -30,6 +30,7 @@ DefineHigherOrderTests([ ['DoubleSome', newClosure('some'), DoubleSetup, v => v < 0.0], ['SmiSome', newClosure('some'), SmiSetup, v => v === 34343], ['FastSome', newClosure('some'), FastSetup, v => v === 'hi'], + ['DictionarySome', newClosure('some'), DictionarySetup, v => v > 0], ['OptFastSome', OptFastSome, FastSetup, undefined], ['OptUnreliableSome', OptUnreliableSome, FastSetup, v => v === 'hi'] ]); diff --git a/test/js-perf-test/JSTests.json b/test/js-perf-test/JSTests.json index c2aacb452f..4b4a7ead15 100644 --- a/test/js-perf-test/JSTests.json +++ b/test/js-perf-test/JSTests.json @@ -695,6 +695,7 @@ {"name": "DoubleForEach"}, {"name": "SmiForEach"}, {"name": "FastForEach"}, + {"name": "DictionaryForEach"}, {"name": "GenericForEach"}, {"name": "OptFastForEach"}, {"name": "OptUnreliableForEach"}, @@ -702,6 +703,7 @@ {"name": "DoubleFilter"}, {"name": "SmiFilter"}, {"name": "FastFilter"}, + {"name": "DictionaryFilter"}, {"name": "GenericFilter"}, {"name": "OptFastFilter"}, {"name": "OptUnreliableFilter"}, @@ -709,12 +711,14 @@ {"name": "DoubleMap"}, {"name": "SmiMap"}, {"name": "FastMap"}, + {"name": "DictionaryMap"}, {"name": "GenericMap"}, {"name": "OptFastMap"}, {"name": "OptUnreliableMap"}, {"name": "DoubleEvery"}, {"name": "SmiEvery"}, {"name": "FastEvery"}, + {"name": "DictionaryEvery"}, {"name": "OptFastEvery"}, {"name": "OptUnreliableEvery"}, {"name": "SmiJoin"}, @@ -724,6 +728,7 @@ {"name": "DoubleSome"}, {"name": "SmiSome"}, {"name": "FastSome"}, + {"name": "DictionarySome"}, {"name": "OptFastSome"}, {"name": "OptUnreliableSome"}, {"name": "DoubleReduce"},