New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 6936 link

Starred by 8 users

Issue metadata

Status: Started
Owner:
Vacation until after Christmas
Cc:
HW: All
NextAction: ----
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 2017

Issue description

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 22 by bugdroid1@chromium.org, Oct 30 2017

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 31 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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

Blockedon: 7137
Project Member

Comment 40 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/99cb4d35a3511d08307bb8012dbb2d2b3de157a4

commit 99cb4d35a3511d08307bb8012dbb2d2b3de157a4
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Wed Nov 29 09:58:07 2017

[cleanup] Harden the SubString CSA/Runtime implementations.

Remove the self-healing for invalid parameters in the
CodeStubAssembler::SubString helper and the %SubString runtime function,
which is used as a fallback for the CodeStubAssembler implementation.
All call sites must do appropriate parameter validation anyways now that
the self-hosted JavaScript builtins using these helpers are gone, and we
have proper contracts with the uses.

Also remove the context parameter from the CodeStubAssembler::SubString
method, which is unnecessary, since this can no longer throw an
exception.

Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
Change-Id: I19d93bad5f41faa0561c4561a48f78fcba99a549
Reviewed-on: https://chromium-review.googlesource.com/795720
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49702}
[modify] https://crrev.com/99cb4d35a3511d08307bb8012dbb2d2b3de157a4/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/99cb4d35a3511d08307bb8012dbb2d2b3de157a4/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/99cb4d35a3511d08307bb8012dbb2d2b3de157a4/src/code-stub-assembler.cc
[modify] https://crrev.com/99cb4d35a3511d08307bb8012dbb2d2b3de157a4/src/code-stub-assembler.h
[modify] https://crrev.com/99cb4d35a3511d08307bb8012dbb2d2b3de157a4/src/runtime/runtime-strings.cc
[modify] https://crrev.com/99cb4d35a3511d08307bb8012dbb2d2b3de157a4/test/cctest/test-strings.cc

Project Member

Comment 41 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd

commit c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd
Author: Michael Achenbach <machenbach@chromium.org>
Date: Wed Nov 29 10:50:28 2017

Revert "[cleanup] Harden the SubString CSA/Runtime implementations."

This reverts commit 99cb4d35a3511d08307bb8012dbb2d2b3de157a4.

Reason for revert:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20debug/builds/16445

Original change's description:
> [cleanup] Harden the SubString CSA/Runtime implementations.
> 
> Remove the self-healing for invalid parameters in the
> CodeStubAssembler::SubString helper and the %SubString runtime function,
> which is used as a fallback for the CodeStubAssembler implementation.
> All call sites must do appropriate parameter validation anyways now that
> the self-hosted JavaScript builtins using these helpers are gone, and we
> have proper contracts with the uses.
> 
> Also remove the context parameter from the CodeStubAssembler::SubString
> method, which is unnecessary, since this can no longer throw an
> exception.
> 
> Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
> Change-Id: I19d93bad5f41faa0561c4561a48f78fcba99a549
> Reviewed-on: https://chromium-review.googlesource.com/795720
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49702}

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

Change-Id: I2900b5f087e78f1d321724f03bd063a5ff094183
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
Reviewed-on: https://chromium-review.googlesource.com/796150
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49703}
[modify] https://crrev.com/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd/src/code-stub-assembler.cc
[modify] https://crrev.com/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd/src/code-stub-assembler.h
[modify] https://crrev.com/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd/src/runtime/runtime-strings.cc
[modify] https://crrev.com/c0a4680d701b1994dbdd32ca2a35cc5510ba6ecd/test/cctest/test-strings.cc

Project Member

Comment 42 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3200cc600cd5f655ed638adcc2b73ad8eb9ef26a

commit 3200cc600cd5f655ed638adcc2b73ad8eb9ef26a
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Wed Nov 29 10:55:26 2017

[turbofan] Optimize String#slice(-1) calls.

In TurboFan we can easily recognize calls to String.prototype.slice
where the start parameter is -1 and the end parameter is either
undefined or not present. These calls either return an empty string if
the input string is empty, or the last character of the input string
as a single character string. So we can just make use of the existing
StringCharAt operator.

This reduces the overhead of the String.prototype.slice calls from
optimized code in the chai test of the web-tooling-benchmark
significantly. We observe a 2-3% improvement on the test.

Bug: v8:6936, v8:7137
Change-Id: Iebe02667446880f5760e3e8c80f8b7cc712df663
Reviewed-on: https://chromium-review.googlesource.com/795726
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49704}
[modify] https://crrev.com/3200cc600cd5f655ed638adcc2b73ad8eb9ef26a/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/3200cc600cd5f655ed638adcc2b73ad8eb9ef26a/src/compiler/js-builtin-reducer.h
[add] https://crrev.com/3200cc600cd5f655ed638adcc2b73ad8eb9ef26a/test/mjsunit/compiler/string-slice.js

Project Member

Comment 43 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68

commit c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 30 04:27:15 2017

[turbofan] Introduce dedicated NewConsString operator.

This is in preparation of adding a dedicated StringLength operator that
loads the string length. This way operations on strings don't sit in the
effect chain anymore until the EffectControlLinearizer, which wires them.

The NewConsString semantics could still be better, i.e. it could try to
figure out the proper map instead of going for the CONS_STRING_TYPE
always. But this change is meant to be just about pushing the logic down
to the EffectControlLinearizer, which we didn't have initially when the
ConsString handling was done.

This also allows us to remove the handling of CONS_STRING_TYPE from the
Deoptimizer, since the escape analysis no longer sees cons strings.

Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
Change-Id: If6c4a6d7cf63a3a3f7a34a920c8e50a94dfa67fa
Reviewed-on: https://chromium-review.googlesource.com/796413
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49729}
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/effect-control-linearizer.cc
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/effect-control-linearizer.h
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/js-typed-lowering.cc
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/opcodes.h
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/simplified-operator.cc
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/simplified-operator.h
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/typer.cc
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/compiler/verifier.cc
[modify] https://crrev.com/c061734f399e0e7bb3a8f7e3a3797dadcc0eaa68/src/deoptimizer.cc

Project Member

Comment 44 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/500d7b93153e5fb14a212ac4a87183a2a680b736

commit 500d7b93153e5fb14a212ac4a87183a2a680b736
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 30 06:55:06 2017

[turbofan] Introduce a dedicated StringLength operator.

Strings are immutable in JavaScript land (contrast with the runtime,
where we can truncate strings that haven't escaped to JavaScript yet),
so the length of a String is immutable. Thus loading the length of a
String is a pure operation and should be expressed as such (i.e. doesn't
depend on control or effect). The StringLength operator does exactly
this and is hooked up to the effect chain in the EffectControlLinearizer.

This will eventually allow us to simplify the optimization of string
concatention and other operations that are a bit cumbersome in TurboFan
currently, and it will also allow us to optimize string operations
across effectful operations, for example combining multiple invocations
to String#slice with the same inputs.

Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
Change-Id: Iffcccbb0c7fc4cfe1281c10e7af24b40eba4c987
Reviewed-on: https://chromium-review.googlesource.com/799690
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49731}
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/effect-control-linearizer.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/effect-control-linearizer.h
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/js-typed-lowering.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/js-typed-lowering.h
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/opcodes.h
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/simplified-operator.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/simplified-operator.h
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/typer.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/src/compiler/verifier.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/test/unittests/compiler/js-typed-lowering-unittest.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/test/unittests/compiler/node-test-utils.cc
[modify] https://crrev.com/500d7b93153e5fb14a212ac4a87183a2a680b736/test/unittests/compiler/node-test-utils.h

Project Member

Comment 45 by bugdroid1@chromium.org, Feb 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/bb282636cf2d61915781c66dfeed0fe6cc3614ae

commit bb282636cf2d61915781c66dfeed0fe6cc3614ae
Author: Sigurd Schneider <sigurds@chromium.org>
Date: Tue Feb 13 09:07:35 2018

Reland "[cleanup] Harden the SubString CSA/Runtime implementations."

This is a reland of 6d5b54df82e27a82811a836dcdbbfe26829f0e6d
Original change's description:
> [cleanup] Harden the SubString CSA/Runtime implementations.
>
> Remove the self-healing for invalid parameters in the
> CodeStubAssembler::SubString helper and the %SubString runtime function,
> which is used as a fallback for the CodeStubAssembler implementation.
> All call sites must do appropriate parameter validation anyways now that
> the self-hosted JavaScript builtins using these helpers are gone, and we
> have proper contracts with the uses.
>
> Also remove the context parameter from the CodeStubAssembler::SubString
> method, which is unnecessary, since this can no longer throw an
> exception.
>
> Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
> Change-Id: I19d93bad5f41faa0561c4561a48f78fcba99a549
> Reviewed-on: https://chromium-review.googlesource.com/795720
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49702}

Bug: v8:5269, v8:6936,  v8:7109 , v8:7137
Change-Id: I5e84998a2dd3990d7981505b401ffc770e0b7ac5
Reviewed-on: https://chromium-review.googlesource.com/913130
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51265}
[modify] https://crrev.com/bb282636cf2d61915781c66dfeed0fe6cc3614ae/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/bb282636cf2d61915781c66dfeed0fe6cc3614ae/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/bb282636cf2d61915781c66dfeed0fe6cc3614ae/src/code-stub-assembler.cc
[modify] https://crrev.com/bb282636cf2d61915781c66dfeed0fe6cc3614ae/src/code-stub-assembler.h
[modify] https://crrev.com/bb282636cf2d61915781c66dfeed0fe6cc3614ae/src/runtime/runtime-strings.cc
[modify] https://crrev.com/bb282636cf2d61915781c66dfeed0fe6cc3614ae/test/cctest/test-strings.cc

Project Member

Comment 46 by bugdroid1@chromium.org, Sep 11

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a15ad0d310624783ebbf5bd0e41536f64ca45969

commit a15ad0d310624783ebbf5bd0e41536f64ca45969
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Sep 11 18:04:01 2018

[turbofan] Reduce overhead of megamorphic property accesses.

We had an optimization in Crankshaft where we would call into the
megamorphic handler stub directly if an inline cache was already
found to be megamorphic when it hit the optimizing compiler. This
way we could avoid the dispatch overhead when we know that there's
no point in checking for the other states anyways. However we somehow
missed to port this optimization to TurboFan.

Now this change introduces support to call into LoadIC_Megamorphic and
KeyedLoadIC_Megamorphic directly (plus the trampoline versions), which
saves quite a lot of overhead for the cases where the map/name pair is
found in the megamorphic stub cache, and it's quite a simple change. We
can later extend this to also handle the StoreIC and KeyedStoreIC cases
if that turns out to be beneficial.

This improves the score on the Octane/TypeScript test by around ~2%
and the TypeScript test in the web-tooling-benchmark by around ~4%. On
the ARES-6 Air test the steady state mean improves by 2-4%, and on the
ARES-6 ML test the steady state mean seems to also improve by 1-2%, but
that might be within noise.

On a micro-benchmark that just runs `o.x` in a hot loop on a set of 9
different objects, which all have `x` as the first property and are
all in fast mode, we improve by around ~30%, and are now almost on par
with JavaScriptCore.

Bug: v8:6344, v8:6936
Change-Id: Iaa4c6e34c37e78da217ee75f32f6acc95a834250
Reviewed-on: https://chromium-review.googlesource.com/1215623
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55803}
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/builtins/builtins-definitions.h
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/builtins/builtins-ic-gen.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/compiler/bytecode-graph-builder.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/compiler/js-generic-lowering.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/compiler/js-operator.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/feedback-vector.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/globals.h
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/ic/accessor-assembler.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/ic/accessor-assembler.h
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/objects-printer.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/vector-slot-pair.cc
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/src/vector-slot-pair.h
[modify] https://crrev.com/a15ad0d310624783ebbf5bd0e41536f64ca45969/test/unittests/compiler/js-call-reducer-unittest.cc

Project Member

Comment 47 by bugdroid1@chromium.org, Oct 7

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/bcdede0c53ab6c0cb6912111f599435cd8f6929f

commit bcdede0c53ab6c0cb6912111f599435cd8f6929f
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Sun Oct 07 12:00:01 2018

[turbofan] Eliminate redundant Smi checks around array accesses.

As identified in the web-tooling-benchmark, there are specific code
patterns involving array indexed property accesses and subsequent
comparisons of those indices that lead to repeated Smi checks in the
optimized code, which in turn leads to high register pressure and
generally bad register allocation. An example of this pattern is
code like this:

```js
function f(a, n) {
  const i = a[n];
  if (n >= 1) return i;
}
```

The `a[n]` property access introduces a CheckBounds on `n`, which
later lowers to a `CheckedTaggedToInt32[dont-check-minus-zero]`,
however the `n >= 1` comparison has collected `SignedSmall` feedback
and so it introduces a `CheckedTaggedToTaggedSigned` operation. This
second Smi check is redundant and cannot easily be combined with the
earlier tagged->int32 conversion, since that also deals with heap
numbers and even truncates -0 to 0.

So we teach the RedundancyElimination to look at the inputs of these
speculative number comparisons and if there's a leading bounds check
on either of these inputs, we change the input to the result of the
bounds check. This avoids the redundant Smi checks later and generally
allows the SimplifiedLowering to do a significantly better job on the
number comparisons. We only do this in case of SignedSmall feedback
and only for inputs that are not already known to be in UnsignedSmall
range, to avoid doing too many (unnecessary) expensive lookups during
RedundancyElimination.

All of this is safe despite the fact that CheckBounds truncates -0
to 0, since the regular number comparisons in JavaScript identify
0 and -0 (unlike Object.is()). This also adds appropriate tests,
especially for the interesting cases where -0 is used only after
the code was optimized.

Bug: v8:6936,  v8:7094 
Change-Id: Ie37114fb6192e941ae1a4f0bfe00e9c0a8305c07
Reviewed-on: https://chromium-review.googlesource.com/c/1246181
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56428}
[modify] https://crrev.com/bcdede0c53ab6c0cb6912111f599435cd8f6929f/src/compiler/redundancy-elimination.cc
[modify] https://crrev.com/bcdede0c53ab6c0cb6912111f599435cd8f6929f/src/compiler/redundancy-elimination.h
[modify] https://crrev.com/bcdede0c53ab6c0cb6912111f599435cd8f6929f/test/mjsunit/compiler/redundancy-elimination.js
[modify] https://crrev.com/bcdede0c53ab6c0cb6912111f599435cd8f6929f/test/unittests/compiler/node-test-utils.cc
[modify] https://crrev.com/bcdede0c53ab6c0cb6912111f599435cd8f6929f/test/unittests/compiler/node-test-utils.h
[modify] https://crrev.com/bcdede0c53ab6c0cb6912111f599435cd8f6929f/test/unittests/compiler/redundancy-elimination-unittest.cc

Sign in to add a comment