New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix OOB string character access in Printer#_maybeAddParen. #6589

Merged
merged 2 commits into from Oct 30, 2017

Conversation

@bmeurer
Member

bmeurer commented Oct 29, 2017

The _maybeAddParen method of the Printer class does

const chaPost = str[i + 1]

without checking that i + 1 is still within the bounds of str. It
seems like this triggers fairly often that the str[i + 1] access is
out of bounds. The first out of bounds access will turn the KeyedLoadIC
(in case of V8) into MEGAMORPHIC state, which is significantly slower
for strings (there's a fix in flight for V8 to mitigate the cost a bit
in that case). Even worse than that, the out of bounds access also
pollutes the later comparisons, namely

chaPost === "/"

and

chaPost === "*"

which are now no longer monomorphic on strings, since chaPost was
sometimes undefined.

This is a non-breaking performance fix, which improves babel execution
on the web-tooling-benchmark
workload by around 6-9%.

Fix OOB string character access in Printer#_maybeAddParen.
The `_maybeAddParen` method of the `Printer` class does

```js
const chaPost = str[i + 1]
```

without checking that `i + 1` is still within the bounds of `str`. It
seems like this triggers fairly often that the `str[i + 1]` access is
out of bounds. The first out of bounds access will turn the KeyedLoadIC
(in case of V8) into *MEGAMORPHIC* state, which is significantly slower
for strings (there's a fix in flight for V8 to mitigate the cost a bit
in that case). Even worse than that, the out of bounds access also
pollutes the later comparisons, namely

```js
chaPost === "/"
```

and

```js
chaPost === "*"
```

which are now no longer monomorphic on strings, since `chaPost` was
sometimes `undefined`.

This is a non-breaking performance fix, which improves babel execution
on the [web-tooling-benchmark](github.com/v8/web-tooling-benchmark)
workload by around 6-9%.
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 29, 2017

Member

Tests are failing seems like via browserlist (saw a release 18 min ago)? browserslist/browserslist@fc5bfb9 @ai is this a regression or what we did?

  1. getTargets parses:
    TypeError: Path must be a string. Received undefined
    at assertPath (path.js:28:11)
    at Object.resolve (path.js:1186:7)
    at Function.browserslist.findConfig (experimental/babel-preset-env/node_modules/browserslist/index.js:384:15)
    at browserslist (experimental/babel-preset-env/node_modules/browserslist/index.js:234:38)
    at getTargets (experimental/babel-preset-env/src/targets-parser.js:110:46)
    at Context. (experimental/babel-preset-env/test/targets-parser.spec.js:11:58)
Member

hzoo commented Oct 29, 2017

Tests are failing seems like via browserlist (saw a release 18 min ago)? browserslist/browserslist@fc5bfb9 @ai is this a regression or what we did?

  1. getTargets parses:
    TypeError: Path must be a string. Received undefined
    at assertPath (path.js:28:11)
    at Object.resolve (path.js:1186:7)
    at Function.browserslist.findConfig (experimental/babel-preset-env/node_modules/browserslist/index.js:384:15)
    at browserslist (experimental/babel-preset-env/node_modules/browserslist/index.js:234:38)
    at getTargets (experimental/babel-preset-env/src/targets-parser.js:110:46)
    at Context. (experimental/babel-preset-env/test/targets-parser.spec.js:11:58)
@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Oct 29, 2017

Contributor

I just released 2.6, but I changed only ./cli.js. You don’t use browserslist CLI in babel-preset-env?

What was value of path option?

Contributor

ai commented Oct 29, 2017

I just released 2.6, but I changed only ./cli.js. You don’t use browserslist CLI in babel-preset-env?

What was value of path option?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 29, 2017

Member

Not sure it should have the lock file anyway which is weird, just didn't see it fail on master and nothing change otherwise for us

Member

hzoo commented Oct 29, 2017

Not sure it should have the lock file anyway which is weird, just didn't see it fail on master and nothing change otherwise for us

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Oct 29, 2017

Contributor

I found the problem. Release will be soon.

Contributor

ai commented Oct 29, 2017

I found the problem. Release will be soon.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Oct 29, 2017

Contributor

Fix was released in 2.6.1

Contributor

ai commented Oct 29, 2017

Fix was released in 2.6.1

@hzoo hzoo requested review from existentialism and loganfsmyth Oct 29, 2017

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Oct 29, 2017

Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5531/

Collaborator

babel-bot commented Oct 29, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5531/

@xtuc

xtuc approved these changes Oct 30, 2017

👍

@hzoo hzoo merged commit 0034245 into babel:master Oct 30, 2017

3 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kisg pushed a commit to paul99/v8mips that referenced this pull request Oct 31, 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

  babel/babel#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}

@hzoo hzoo added the PR: Polish 💅 label Nov 6, 2017

@lock lock bot added the outdated label May 1, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 1, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.