New issue
Advanced search Search tips
Starred by 2 users
Status: Fixed
Owner:
Closed: Nov 2
Cc:
Components:
HW: All
OS: All
Priority: 1
Type: Bug

Blocking:
issue 6936



Sign in to add a comment
MEGAMORPHIC KeyedLoadIC's fall back to %KeyedGetProperty to load string characters
Project Member Reported by bmeu...@chromium.org, Oct 29 Back to list
This was discovered in the babel test of the web-tooling-benchmark. The MEGAMORPHIC KeyedLoadIC handler falls back to the slow runtime path for all instance types less than or equal to LAST_CUSTOM_ELEMENTS_RECEIVER, which includes all strings. This is unecessary since we could just as well tail call to the StringCharAt builtin if the index is within bounds. This will significantly reduce the number of %KeyedGetProperty calls in babel.
 
Looks like this fix reduces the overhead of %KeyedGetProperty from around 9% to just 2% on the current babel test (not including the latest performance fixes to Babel). Overall it seems to improve number of runs on the babel test by around 7-8%.
Cc: ishell@chromium.org jkummerow@chromium.org
Cc: jarin@chromium.org
Project Member Comment 4 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 5 by bugdroid1@chromium.org, Oct 30
Project Member Comment 6 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

Status: Fixed
Project Member Comment 8 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 9 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

Sign in to add a comment