New issue
Advanced search Search tips
Starred by 5 users
Status: Started
Owner:
Cc:
HW: All
OS: All
Priority: 2
Type: FeatureRequest


Sign in to add a comment
Optimize web-tooling-benchmark
Project Member Reported by bmeu...@chromium.org, Oct 16 Back to list
Benchmark: http://benediktmeurer.de/web-tooling-benchmark
Backlog: https://goo.gl/wQG6dq

This is the tracking bug for the issues discovered in the web-tooling-benchmark. We will collect interesting findings in the backlog document.
 
Blockedon: 6937
Project Member Comment 2 by bugdroid1@chromium.org, Oct 16
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/4213af64b6b9c9d04be89303169562460d38df84

commit 4213af64b6b9c9d04be89303169562460d38df84
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Oct 16 17:27:41 2017

[es2015] Optimize Reflect.has builtin.

Port the baseline version of Reflect.has to the CodeStubAssembler and
reuse the existing logic for HasProperty (i.e. the HasProperty builtin).
Also inline the Reflect.has builtin into TurboFan, by adding a check
on the target in front of a use of the JSHasProperty operator.
Technically this additional check is not necessary, because the
JSHasProperty operator already throws if the target is not a JSReceiver,
but the exception message is confusing then.

This improves the performance of the micro-benchmark from

  reflectHasPresent: 337 ms.
  reflectHasAbsent: 472 ms.

to

  reflectHasPresent: 121 ms.
  reflectHasAbsent: 216 ms.

which is a nice 2.8x improvement in the best case. It also improves the
chai test on the web-tooling-benchmark by around 1-2%, which is roughly
the expected win (since Reflect.has overall accounts for around 3-4%).

Bug: v8:5996, v8:6936,  v8:6937 
Change-Id: I856183229677a71c19936f06f2a4fc7a794a9a4a
Reviewed-on: https://chromium-review.googlesource.com/720959
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48608}
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/BUILD.gn
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-definitions.h
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-promise-gen.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-promise-gen.h
[add] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-reflect-gen.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-reflect.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/code-stub-assembler.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/code-stub-assembler.h
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/compiler/js-call-reducer.h
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/v8.gyp
[add] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/test/mjsunit/compiler/reflect-has.js

Blockedon: 6946
Blockedon: 6948
Project Member Comment 5 by bugdroid1@chromium.org, Oct 17
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/594803c94671fa19db1420f2b43023b75264e101

commit 594803c94671fa19db1420f2b43023b75264e101
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Oct 17 12:56:32 2017

[turbofan] Inline Function#bind in more cases.

So far the inlining of Function#bind into TurboFan optimized code was
limited to cases where TurboFan could infer the constant JSFunction that
was bound. However we can easily extend that to cover JSBoundFunction as
well, and obviously also take the LOAD_IC feedback if we don't have a
known JSFunction or JSBoundFunction.

This adds a new operator JSCreateBoundFunction that contains the logic
for the creation of the bound function object and the arguments.

On the micro-benchmarks we go from

  functionBindParameter0: 1239 ms.
  functionBindConstant0: 478 ms.
  functionBindBoundConstant0: 1256 ms.
  functionBindParameter1: 1278 ms.
  functionBindConstant1: 475 ms.
  functionBindBoundConstant1: 1253 ms.
  functionBindParameter2: 1431 ms.
  functionBindConstant2: 616 ms.
  functionBindBoundConstant2: 1437 ms.

to

  functionBindParameter0: 462 ms.
  functionBindConstant0: 485 ms.
  functionBindBoundConstant0: 474 ms.
  functionBindParameter1: 478 ms.
  functionBindConstant1: 474 ms.
  functionBindBoundConstant1: 474 ms.
  functionBindParameter2: 617 ms.
  functionBindConstant2: 614 ms.
  functionBindBoundConstant2: 616 ms.

which is a ~2.5x improvement. On the jshint benchmark in the
web-tooling-benchmark we observe a 2-3% improvement, which corresponds
to the time we had seen it running in the generic version.

Bug: v8:6936,  v8:6946 
Change-Id: I940d13220ff35ae602dbaa33349ba4bbe0c9a9d3
Reviewed-on: https://chromium-review.googlesource.com/723080
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48639}
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-builtin-reducer.h
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-call-reducer.h
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-create-lowering.h
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-generic-lowering.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-inlining.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-operator.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/js-operator.h
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/opcodes.h
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/typer.cc
[modify] https://crrev.com/594803c94671fa19db1420f2b43023b75264e101/src/compiler/verifier.cc

Blockedon: 6969
Blockedon: 6971
Project Member Comment 8 by bugdroid1@chromium.org, Oct 20
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe

commit d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Fri Oct 20 12:16:10 2017

[ic] Ensure that we make progress on KeyedLoadIC polymorphic name.

In the special case of KeyedLoadIC, where the key that is passed in is a
Name that is always the same we only checked for identity in both the
stub and the TurboFan case, which works fine for symbols and internalized
strings, but doesn't really work with non-internalized strings, where
the identity check will fail, the runtime will internalize the string,
and the IC will then see the original internalized string again and not
progress in the feedback lattice. This leads to tricky deoptimization
loops in TurboFan and constantly missing ICs.

This adds fixes the stub to always try to internalize strings first
when the identity check fails and then doing the check again. If the
name is not found in the string table we miss, since in that case the
string cannot match the previously recorded feedback name (which is
always a unique name).

In TurboFan we represent this checks with new CheckEqualsSymbol and
CheckEqualsInternalizedString operators, which validate the previously
recorded feedback, and the CheckEqualsInternalizedString operator does
the attempt to internalize the input.

Bug: v8:6936,  v8:6948 ,  v8:6969 
Change-Id: I3f3b4a587c67f00f7c4b60d239eb98a9626fe04a
Reviewed-on: https://chromium-review.googlesource.com/730224
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48784}
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/builtins/builtins-definitions.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/builtins/builtins-ic-gen.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/c-linkage.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/effect-control-linearizer.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/effect-control-linearizer.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/js-native-context-specialization.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/opcodes.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/simplified-operator.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/simplified-operator.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/typed-optimization.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/typed-optimization.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/typer.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/compiler/verifier.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/deoptimize-reason.h
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/ic/accessor-assembler.cc
[modify] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/src/ic/accessor-assembler.h
[add] https://crrev.com/d5c19aa9fc536638b4d3831ba01cf0f1a6a05bbe/test/mjsunit/regress/regress-6948.js

Blockedon: 6978
Blockedon: 6980
Project Member Comment 11 by bugdroid1@chromium.org, Oct 23
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/bcee140617fe90e8c354394216225615b9558113

commit bcee140617fe90e8c354394216225615b9558113
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Oct 23 10:15:36 2017

[turbofan] Introduce InstanceOfIC to collect rhs feedback.

This adds a new InstanceOfIC where the TestInstanceOf bytecode collects
constant feedback about the right-hand side of instanceof operators,
including both JSFunction and JSBoundFunction instances. TurboFan then
uses the feedback to optimize instanceof in places where the right-hand
side is not a known constant (known to TurboFan).

This addresses the odd performance cliff that we see with instanceof in
functions with multiple closures. It was discovered as one of the main
bottlenecks on the uglify-es test in the web-tooling-benchmark. The
uglify-es test (run in separation) is ~18% faster with this change.

On the micro-benchmark in the tracking bug we go from

  instanceofSingleClosure_Const: 69 ms.
  instanceofSingleClosure_Class: 246 ms.
  instanceofMultiClosure: 246 ms.
  instanceofParameter: 246 ms.

to

  instanceofSingleClosure_Const: 70 ms.
  instanceofSingleClosure_Class: 75 ms.
  instanceofMultiClosure: 76 ms.
  instanceofParameter: 73 ms.

boosting performance by roughly 3.6x and thus effectively removing the
performance cliff around instanceof.

Bug: v8:6936,  v8:6971 
Change-Id: Ib88dbb9eaef9cafa4a0e260fbbde73427a54046e
Reviewed-on: https://chromium-review.googlesource.com/730686
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48820}
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/bytecode-graph-builder.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/js-operator.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/js-operator.h
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/js-type-hint-lowering.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/property-access-builder.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/compiler/property-access-builder.h
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/feedback-vector-inl.h
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/feedback-vector.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/feedback-vector.h
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/interpreter/bytecode-array-builder.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/interpreter/bytecode-generator.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/interpreter/bytecodes.h
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/interpreter/interpreter-assembler.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/interpreter/interpreter-assembler.h
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/interpreter/interpreter-generator.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/src/objects-printer.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/test/cctest/interpreter/bytecode_expectations/IfConditions.golden
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/test/cctest/interpreter/test-interpreter.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/test/mjsunit/compiler/instanceof.js
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/test/unittests/compiler/typer-unittest.cc
[modify] https://crrev.com/bcee140617fe90e8c354394216225615b9558113/test/unittests/interpreter/bytecode-array-builder-unittest.cc

Project Member Comment 12 by bugdroid1@chromium.org, Oct 23
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/35614b7215ed56a8653787fadfba4609b1237733

commit 35614b7215ed56a8653787fadfba4609b1237733
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Oct 23 17:42:38 2017

[turbofan] Optimize Reflect.get(target, key) calls.

When TurboFan sees a call to Reflect.get with exactly two parameters,
we can lower that to a direct call to the GetPropertyStub, which is
certainly faster than the general C++ builtin. This gives a nice
7-8% improvement on the chai test in the web-tooling-benchmark.

The micro-benchmark on the issue goes from

  reflectGetPresent: 461 ms.
  reflectGetAbsent: 470 ms.

to 

  reflectGetPresent: 141 ms.
  reflectGetAbsent: 245 ms.

which is an up to 3.2x improvement.

Bug: v8:5996, v8:6936,  v8:6937 
Change-Id: Ic439fccb13f1a2f84386bf9fc31b4283d101afc4
Reviewed-on: https://chromium-review.googlesource.com/732988
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48841}
[modify] https://crrev.com/35614b7215ed56a8653787fadfba4609b1237733/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/35614b7215ed56a8653787fadfba4609b1237733/src/compiler/js-call-reducer.h
[add] https://crrev.com/35614b7215ed56a8653787fadfba4609b1237733/test/mjsunit/compiler/reflect-get.js

Blockedon: 6985
Blockedon: 6988
Blockedon: 6991
Project Member Comment 16 by bugdroid1@chromium.org, Oct 24
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a9b098013d18fbf285c45edb3d29b0fa646633ac

commit a9b098013d18fbf285c45edb3d29b0fa646633ac
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Oct 24 08:42:32 2017

[ic] Improve KeyedStoreIC performance for dictionaries.

Once the KeyedStoreIC was in MEGAMORPHIC state storing to dictionary
mode objects, we'd constantly hit the slow-path implemented via the
%SetProperty runtime function, if the dictionary was created with a
null prototype, i.e. via Object.create(null). This goes against the
advice of using Object.create(null) for dictionaries (compared to
using empty object literal), which is unfortunate.

This CL addresses two issues, starting with

- adding support for null prototypes to LookupPropertyOnPrototypeChain,
  which was always hitting the slow path for null prototypes, and
- using the dedicated %AddDictionaryProperty runtime call when we
  have to grow the backing store.

These changes combined improve the micro-benchmark from

  storeToDictionary: 559 ms.
  storeToFast: 95 ms.

to

  storeToDictionary: 201 ms.
  storeToFast: 94 ms.

which reduces overhead by about 65%. This overall improves the chai test
on the web-tooling-benchmark by about 4%, which still leaves some room
for improvement.

Bug: v8:6936,  v8:6985 
Change-Id: I97b78961f51edb3a3e198bdb31457fd78bed947f
Reviewed-on: https://chromium-review.googlesource.com/735139
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48858}
[modify] https://crrev.com/a9b098013d18fbf285c45edb3d29b0fa646633ac/src/ic/keyed-store-generic.cc

Project Member Comment 17 by bugdroid1@chromium.org, Oct 24
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a9da0ce73565a5b4357548686590cd0ae3735f68

commit a9da0ce73565a5b4357548686590cd0ae3735f68
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Oct 24 09:19:47 2017

[turbofan] Properly handle smis in monomorphic loads/stores.

When lowering a monomorphic load/store, where multiple receiver maps
have been recorded, but the action to be performed is the same (i.e.
yielding undefined because the property is not found), TurboFan used
to ignore the Smi case, leading to a pretty terrible deoptimization
loop, as the LOAD_IC/STORE_IC properly recorded that state and thus
didn't change it's state.

Fixing this issue gives a 18-20% boost on the prettier test of the
web-tooling-benchmark, which was suffering a lot from this problem.

Bug: v8:6936,  v8:6991 
Change-Id: Id208ec7129a7f6b190d989bda31f936040393226
Reviewed-on: https://chromium-review.googlesource.com/735342
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48865}
[modify] https://crrev.com/a9da0ce73565a5b4357548686590cd0ae3735f68/src/compiler/js-native-context-specialization.cc
[add] https://crrev.com/a9da0ce73565a5b4357548686590cd0ae3735f68/test/mjsunit/regress/regress-6991.js

Blockedon: 6997
Blockedon: 7011
Blockedon: 7014
Project Member Comment 21 by bugdroid1@chromium.org, Oct 30
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8bb392d21156c9266d35294a0aec4a10e51def51

commit 8bb392d21156c9266d35294a0aec4a10e51def51
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Oct 30 07:46:45 2017

[ic] Handle String character loads in KeyedLoadIC_Megamorphic.

This was discovered on the babel test of the web-tooling-benchmark,
which suffers from a high %KeyedGetProperty overhead, and most of
these calls come from the fact that the KeyedLoadIC_Megamorphic bails
out to the runtime call for all String instance types. Just handling
in-bound accesses to characters reduces the overhead incurred by
%KeyedGetProperty from roughly 9% to roughly 2% only.

This reduces the number of runs per second on the babel test by around
7-8% on average.

Bug: v8:6936,  v8:7014 
Change-Id: I0dc247d7d6457c7032636d2852cb54cef1b24979
Reviewed-on: https://chromium-review.googlesource.com/743012
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49021}
[modify] https://crrev.com/8bb392d21156c9266d35294a0aec4a10e51def51/src/ic/accessor-assembler.cc

Project Member Comment 23 by bugdroid1@chromium.org, Oct 31
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6dc35ab46f69073db50bb765ca314cb8a279fba5

commit 6dc35ab46f69073db50bb765ca314cb8a279fba5
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Oct 31 11:25:53 2017

[ic] Add OOB support to KeyedLoadIC.

This adds support to the KeyedLoadIC to ignore out of bounds accesses
for Strings and return undefined instead. We add a dedicated bit to the
Smi handler to encode the OOB state and have TurboFan generate appropriate
code for that case as well. This is mostly useful when programs
accidentially access past the length of a string, which was observed and
fixed for example in Babel recently, see

  https://github.com/babel/babel/pull/6589

for details. The idea is to also extend this mechanism to Arrays and
maybe other receivers, as reading beyond the length is also often used
in jQuery and other popular libraries.

Note that this is considered a mitigation for a performance cliff and
not a general optimization of OOB accesses. These should still be
avoided and handled properly instead.

This seems to further improve the babel test on the web-tooling-benchmark
by around 1%, because the OOB access no longer turns the otherwise
MONOMORPHIC access into MEGAMORPHIC state.

Bug: v8:6936,  v8:7014 
Change-Id: I9df03304e056d7001a65da8e9621119f8e9bb55b
Reviewed-on: https://chromium-review.googlesource.com/744022
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49049}
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/bootstrapper.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/compiler/js-native-context-specialization.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/contexts.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/elements.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/feedback-vector.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/feedback-vector.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/ic/accessor-assembler.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/ic/handler-configuration-inl.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/ic/handler-configuration.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/ic/ic.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/ic/ic.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/isolate.cc
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/isolate.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/src/objects.h
[modify] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/test/mjsunit/compiler/deopt-string-outofbounds.js
[add] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/test/mjsunit/regress/regress-7014-1.js
[add] https://crrev.com/6dc35ab46f69073db50bb765ca314cb8a279fba5/test/mjsunit/regress/regress-7014-2.js

Blockedon: 7022
Project Member Comment 25 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f597eec152eb5c5b788c73eaa689553000f5fa36

commit f597eec152eb5c5b788c73eaa689553000f5fa36
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 02 06:39:34 2017

[builtins] Support two byte strings in StringEqual builtin.

This CL adds support for two byte string comparisons to the StringEqual
builtin, which so far was bailing out to the generic %StringEqual
runtime function whenever any two-byte string was involved. This made
comparisons that involved two-byte strings, either comparing them to
one-byte strings or comparing two two-byte strings, up to 3x slower than
if only one-byte strings were involved.

With this change, all direct string (SeqString or ExternalString)
equality checks are roughly on par now, and the weird performance cliff
is gone. On the micro-benchmark from the bug we go from

  stringEqualBothOneByteSeqString: 162 ms.
  stringEqualTwoByteAndOneByteSeqString: 446 ms.
  stringEqualOneByteAndTwoByteSeqString: 438 ms.
  stringEqualBothTwoByteSeqString: 472 ms.

to

  stringEqualBothOneByteSeqString: 151 ms.
  stringEqualTwoByteAndOneByteSeqString: 158 ms.
  stringEqualOneByteAndTwoByteSeqString: 166 ms.
  stringEqualBothTwoByteSeqString: 160 ms.

which is the desired result. On the esprima test of the
web-tooling-benchmark we seem to improve by 1-2%, which corresponds to
the savings of going to the runtime for many StringEqual comparisons.

Drive-by-cleanup: Introduce LoadAndUntagStringLength helper into the CSA
with proper typing to avoid the unnecessary shifts on 64-bit platforms
when keeping the length tagged initially in StringEqual.

Bug: v8:4913, v8:6365, v8:6371, v8:6936, v8:7022
Change-Id: I566f4b80e217513775ffbd35e0480154abf59b27
Reviewed-on: https://chromium-review.googlesource.com/749223
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49067}
[modify] https://crrev.com/f597eec152eb5c5b788c73eaa689553000f5fa36/src/builtins/builtins-array-gen.cc
[modify] https://crrev.com/f597eec152eb5c5b788c73eaa689553000f5fa36/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/f597eec152eb5c5b788c73eaa689553000f5fa36/src/builtins/builtins-string-gen.h
[modify] https://crrev.com/f597eec152eb5c5b788c73eaa689553000f5fa36/src/code-stub-assembler.cc
[modify] https://crrev.com/f597eec152eb5c5b788c73eaa689553000f5fa36/src/code-stub-assembler.h
[add] https://crrev.com/f597eec152eb5c5b788c73eaa689553000f5fa36/test/mjsunit/string-equal.js

Blockedon: 7026
Project Member Comment 27 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6366a010080ec106ad83964060825c0a72c77458

commit 6366a010080ec106ad83964060825c0a72c77458
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 02 10:14:07 2017

[ic] Internalize strings on the fly in KeyedLoadICGeneric.

This turns on the existing --internalize_on_the_fly flag for the
MEGAMORPHIC KeyedLoadIC to properly internalize strings before
looking up the property. This avoids the otherwise taken runtime
call to %KeyedGetProperty, which is definitely slower.

Initially the --internalize_on_the_fly flag was turned off because
internalizing strings on the fly causes too much traffic on the
megamorphic stub cache. We avoid this problem here by not probing
the stub cache in that case, which still gives the benefit of not
having to go to the runtime.

This improves the babylon test on the web-tooling-benchmark by around
2-3% and will probably also help with several tests (like React or
Ember) on the Speedometer benchmark.

If this CL causes trouble (i.e. tanks something important), we can
just turn off the --internalize_on_the_fly flag again.

Bug: v8:6936,  v8:7026 
Change-Id: Ia59a8a3799d9624d831d66b05bae3ecef31cee0a
Reviewed-on: https://chromium-review.googlesource.com/750821
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49072}
[modify] https://crrev.com/6366a010080ec106ad83964060825c0a72c77458/src/flag-definitions.h
[modify] https://crrev.com/6366a010080ec106ad83964060825c0a72c77458/src/ic/accessor-assembler.cc

Project Member Comment 28 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e06c116379cc2386d584f6488d673de40248fcc7

commit e06c116379cc2386d584f6488d673de40248fcc7
Author: Michael Achenbach <machenbach@chromium.org>
Date: Thu Nov 02 13:37:35 2017

Revert "[ic] Internalize strings on the fly in KeyedLoadICGeneric."

This reverts commit 6366a010080ec106ad83964060825c0a72c77458.

Reason for revert: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/19429

Original change's description:
> [ic] Internalize strings on the fly in KeyedLoadICGeneric.
> 
> This turns on the existing --internalize_on_the_fly flag for the
> MEGAMORPHIC KeyedLoadIC to properly internalize strings before
> looking up the property. This avoids the otherwise taken runtime
> call to %KeyedGetProperty, which is definitely slower.
> 
> Initially the --internalize_on_the_fly flag was turned off because
> internalizing strings on the fly causes too much traffic on the
> megamorphic stub cache. We avoid this problem here by not probing
> the stub cache in that case, which still gives the benefit of not
> having to go to the runtime.
> 
> This improves the babylon test on the web-tooling-benchmark by around
> 2-3% and will probably also help with several tests (like React or
> Ember) on the Speedometer benchmark.
> 
> If this CL causes trouble (i.e. tanks something important), we can
> just turn off the --internalize_on_the_fly flag again.
> 
> Bug: v8:6936,  v8:7026 
> Change-Id: Ia59a8a3799d9624d831d66b05bae3ecef31cee0a
> Reviewed-on: https://chromium-review.googlesource.com/750821
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49072}

TBR=ishell@chromium.org,bmeurer@chromium.org

Change-Id: I5345eb29016ecd6b7788b1b49b2f53992ea82b58
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:6936,  v8:7026 
Reviewed-on: https://chromium-review.googlesource.com/750904
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49077}
[modify] https://crrev.com/e06c116379cc2386d584f6488d673de40248fcc7/src/flag-definitions.h
[modify] https://crrev.com/e06c116379cc2386d584f6488d673de40248fcc7/src/ic/accessor-assembler.cc

Project Member Comment 29 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/96b1fdb27689535be5ac44854813711e77dbff41

commit 96b1fdb27689535be5ac44854813711e77dbff41
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 02 20:57:10 2017

[ic] Internalize strings on the fly in KeyedLoadICGeneric.

This turns on the existing --internalize_on_the_fly flag for the
MEGAMORPHIC KeyedLoadIC to properly internalize strings before
looking up the property. This avoids the otherwise taken runtime
call to %KeyedGetProperty, which is definitely slower.

Initially the --internalize_on_the_fly flag was turned off because
internalizing strings on the fly causes too much traffic on the
megamorphic stub cache. We avoid this problem here by not probing
the stub cache in that case, which still gives the benefit of not
having to go to the runtime.

This improves the babylon test on the web-tooling-benchmark by around
2-3% and will probably also help with several tests (like React or
Ember) on the Speedometer benchmark.

If this CL causes trouble (i.e. tanks something important), we can
just turn off the --internalize_on_the_fly flag again.

Bug: v8:6936,  v8:7026 
Change-Id: If295ed3fd013f8b0ff031f9979e7df21dab817b6
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/751464
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49093}
[modify] https://crrev.com/96b1fdb27689535be5ac44854813711e77dbff41/src/flag-definitions.h
[modify] https://crrev.com/96b1fdb27689535be5ac44854813711e77dbff41/src/ic/accessor-assembler.cc
[add] https://crrev.com/96b1fdb27689535be5ac44854813711e77dbff41/test/mjsunit/regress/regress-7026.js

Blockedon: chromium:781114
Project Member Comment 31 by bugdroid1@chromium.org, Nov 3
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b7168573ed011113746873fd92b71e8bcc9dddb1

commit b7168573ed011113746873fd92b71e8bcc9dddb1
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Fri Nov 03 07:35:14 2017

[turbofan] Generalized OOB support for KeyedLoadIC.

This extends the support in TurboFan and the ICs for OOB loads to also
apply to typed arrays and receivers whose prototype chain is protected
by the "no elements" protector (aka the Array protector). TurboFan will
generate code to materialize undefined instead when it sees a load that
has the OOB bit set and add an appropriate code dependency on the global
protector. For typed arrays it doesn't even need to check the global
protector since elements are never looked up in the prototype chain
for typed arrays.

In the simple micro-benchmark from the bug we go from

  testInBounds: 103 ms.
  testOutOfBounds: 289 ms.

to

  testInBounds: 103 ms.
  testOutOfBounds: 102 ms.

which fixes the 3x slowdown and thus addresses the performance cliff. In
general it's still beneficial to make sure that you don't access out of
bounds, especially once we introduce a bounds check elimination pass to
TurboFan.

This also seems to improve the jQuery benchmark on the Speedometer test
suite by like 1-2% on average. And the SixSpeed rest benchmarks go from

  rest-es5: 25 ms.
  rest-es6: 23 ms.

to

  rest-es5: 6 ms.
  rest-es6: 4 ms.

so a solid 5.7x improvement there.

Bug: v8:6936,  v8:7014 ,  v8:7027 
Change-Id: Ie99699c69cc40057512e72fd40ae28107216c423
Reviewed-on: https://chromium-review.googlesource.com/750089
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49095}
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/code-stub-assembler.cc
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/compiler/js-native-context-specialization.h
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/feedback-vector.cc
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/ic/accessor-assembler.cc
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/ic/handler-configuration-inl.h
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/ic/handler-configuration.cc
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/ic/handler-configuration.h
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/ic/ic.cc
[modify] https://crrev.com/b7168573ed011113746873fd92b71e8bcc9dddb1/src/ic/ic.h

Project Member Comment 32 by bugdroid1@chromium.org, Nov 3
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/113527b6bc4e489e2ff23ec8d0b30cbabbf5ebd0

commit 113527b6bc4e489e2ff23ec8d0b30cbabbf5ebd0
Author: Tobias Tebbi <tebbi@chromium.org>
Date: Fri Nov 03 09:25:45 2017

[ic] Internalize strings on the fly in KeyedStoreICGeneric

Internalizing a key in the KeyedStoreICGeneric avoids an expensive SetProperty runtime call. 
This improves the prepack benchmark by ~5%. 
In the micro-benchmark copy-object.js attached to the bug, it surfaces as a ~2.5x improvement.
The performance improvement currently relies on the stub cache, since we don't search for 
transitions from within the CSA. As this CL puts additional stress on the stub cache, 
performance regressions wouldn't be too surprising.

Bug: v8:6936,  v8:6997 
Change-Id: Id1469499a3ae5450519ff40d3c5a0915c6de0d45
Reviewed-on: https://chromium-review.googlesource.com/749951
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49097}
[modify] https://crrev.com/113527b6bc4e489e2ff23ec8d0b30cbabbf5ebd0/src/ic/keyed-store-generic.cc

Project Member Comment 33 by bugdroid1@chromium.org, Nov 4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fd150c79884f4ae522dd5c80350e90f4d8a7a2ba

commit fd150c79884f4ae522dd5c80350e90f4d8a7a2ba
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Sat Nov 04 12:06:31 2017

[turbofan] Generate the correct bounds when the array protector isn't valid.

The condition for bounds check generation was not in sync with the
condition that was used for the actual access, which lead to invalid
memory accesses when the array protector was invalid.

Tbr: tebbi@chromium.org
Bug:  chromium:781506 ,  chromium:781494 ,  chromium:781457 ,  chromium:781285 ,  chromium:781381 ,  chromium:781380 , v8:6936,  v8:7014 ,  v8:7027 
Change-Id: Ia5b2ad02940292572ed9b37abd3f9ffaa6d7a26b
Reviewed-on: https://chromium-review.googlesource.com/753590
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49124}
[modify] https://crrev.com/fd150c79884f4ae522dd5c80350e90f4d8a7a2ba/src/compiler/js-native-context-specialization.cc
[add] https://crrev.com/fd150c79884f4ae522dd5c80350e90f4d8a7a2ba/test/mjsunit/regress/regress-crbug-781506-1.js
[add] https://crrev.com/fd150c79884f4ae522dd5c80350e90f4d8a7a2ba/test/mjsunit/regress/regress-crbug-781506-2.js
[add] https://crrev.com/fd150c79884f4ae522dd5c80350e90f4d8a7a2ba/test/mjsunit/regress/regress-crbug-781506-3.js

Blockedon: 7091
Blockedon: 7092
Blockedon: 7094
Project Member Comment 37 by bugdroid1@chromium.org, Nov 20 (4 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a9a1671345fa28c23856cc15e73ed24af400dd2a

commit a9a1671345fa28c23856cc15e73ed24af400dd2a
Author: Benedikt Meurer <bmeurer@google.com>
Date: Mon Nov 20 09:43:35 2017

[cleanup] Rename "array protector" to "no elements protector".

The "array protector" now guards the Object.prototype, the
Array.prototype and the String.prototype, so the name was a
bit misleading nowadays. So the new name "no elements protector"
was chosen.

Bug: v8:6936,  v8:7014 ,  v8:7027 
Change-Id: I9a9d7caa2caf0ac9e78cc6658de2f0506970dfa2
Reviewed-on: https://chromium-review.googlesource.com/778162
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49471}
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/builtins/builtins-array-gen.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/builtins/builtins-call-gen.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/code-stub-assembler.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/code-stub-assembler.h
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/elements.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/heap/heap.h
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/heap/setup-heap-internal.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/ic/accessor-assembler.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/ic/ic.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/isolate.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/isolate.h
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/src/objects.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/test/cctest/test-api.cc
[modify] https://crrev.com/a9a1671345fa28c23856cc15e73ed24af400dd2a/tools/v8heapconst.py

Project Member Comment 38 by bugdroid1@chromium.org, Nov 20 (4 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f24a7e758a669eef41c808a5c5bdc6f9a01cc7a9

commit f24a7e758a669eef41c808a5c5bdc6f9a01cc7a9
Author: Benedikt Meurer <bmeurer@google.com>
Date: Mon Nov 20 10:03:11 2017

[cleanup] Remove stale TODO.

The StringEqual builtin handles two byte strings already.

Bug: v8:4913, v8:6365, v8:6371, v8:6936, v8:7022
Change-Id: I6f5a3999ccdce8e9bfcece6e94362c15183bbd8c
Reviewed-on: https://chromium-review.googlesource.com/778883
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49473}
[modify] https://crrev.com/f24a7e758a669eef41c808a5c5bdc6f9a01cc7a9/src/builtins/builtins-string-gen.cc

Sign in to add a comment