Have es2015 rest transform safely use `arguments` by vhf · Pull Request #2833 · babel/babel

vhf · 2015-11-05T09:57:08Z

Hi, I audited some code generated by Babel and noticed that in some cases some improvements could be made to ensure that V8 optimiser (Crankshaft) will not bail out a little too soon. I focused on the use of arguments: using arguments in an unsafe way prevents V8 (and therefore Node.js) to optimise a function. Here is a first patch in the direction of using arguments safely in Babel es2015-transformed code. Although far from perfect in terms of implementation (first time contributor here, still learning Babel internals, happy to receive your comments and/or to chat on slack or by email) this patch will probably give an idea of what could be done, and could be followed by other patches aiming towards the goal of avoiding generation of V8-unoptimisable code.

With this PR I'm only focusing on the very first and easiest part of my findings. In the future, I'll try to fix some other unsafe uses of arguments in every transformation and then try to focus on other possible V8 optimisations.

From what I understood, performances are more important to Babel than prettiness. This patch somewhat impacts readability of es2015-presets generated code. But it also has impact on performance.

Although tests are green and I did my best to adhere to what I think are Babel's codebase best practices, please review very carefully. For example, I was not sure whether to include optimiseLoadStatement definition in visitor (as seen in defaults.js) or to define them outside of visitor's scope.

sebmck · 2015-11-05T17:08:30Z

optimized -> optimised.

vhf · 2015-11-05T23:22:50Z

Thanks for your comments. In this first commit I fixed the optimized typo, removed the useless newExpr temp variable and reduced nesting by factoring out a function. This is almost guaranteed to work.

jamiebuilds · 2015-11-09T09:27:25Z

Is the || arguments[0] === undefined necessary for the v8 optimisation? It seems odd that it would

You're right, it's not. In most cases arguments.length <= 0 || (undefined || arguments[0]) (I suppose that's what you suggest?) behaves the same way, but they are not really equivalent. Consider the following:

var _arguments = {length: 0}; console.log(_arguments.length <= 0 || _arguments[0] === undefined ? undefined : _arguments[0]); console.log(_arguments.length <= 0 || (undefined || _arguments[0]));

First case will return undefined, second case will return true.

…his relic should go

codecov-io · 2015-11-19T13:03:15Z

Current coverage is `89.20%`

Merging #2833 into master will not affect coverage as of 19a0c1d

@@            master   #2833   diff @@
======================================
  Files          214     214       
  Stmts        15466   15461     -5
  Branches         0       0       
  Methods          0       0       
======================================
- Hit          13797   13792     -5
  Partial          0       0       
  Missed        1669    1669

Review entire Coverage Diff as of 19a0c1d

Powered by Codecov. Updated on successful CI builds.

vhf · 2015-11-19T13:18:03Z

I finally fixed my PR. Tests are green, coverage remains unchanged.

jamiebuilds · 2015-11-19T18:47:48Z

Looks good to me, @sebmck ?

sebmck · 2015-11-19T22:41:52Z

Super disappointing that VMs require this :( I'll hopefully get around to reviewing this early next week. Please feel free to ping this issue if I forget. Really sorry for the delay!

eventualbuddha · 2015-11-19T23:24:56Z

Minor style issue: would INDEX be more appropriate than KEY?

Good point. I chose KEY because it was consistent with what I knew from the existing codebase, e.g. 0, 1, 2

If there's a consensus towards INDEX, I'll change it asap.

I would've picked INDEX, but it doesn't really matter either way.

Have es2015 rest transform safely use `arguments`

sebmck · 2015-12-11T14:37:23Z

Thanks for doing this @vhf! Extremely sorry for the delay in merging this.

kuraga · 2015-12-14T08:52:21Z

@vhf Why not arguments.length <= 0 ? undefined : arguments[0]?

Good catch! The reason behind this code is this code. But you're absolutely right, it's uselessly complicated here.

chpio · 2015-12-20T00:31:30Z

my KEY gets transformed to NaN:

function func(...sources) {
    for (let i = 0; i < sources.length; i += 1) {
        const source = sources[i];
        // ...
    }
}

function func() {
    for (var i = 0; i < arguments.length; i += 1) {
        var source = arguments.length <= NaN || arguments[NaN] === undefined ? undefined : arguments[NaN];
        // ...
    }
}

vhf · 2015-12-20T00:37:46Z

Should be

function func() {
  for (var _len = arguments.length, sources = Array(_len), _key = 0; _key < _len; _key++) {
    sources[_key] = arguments[_key];
  }

  for (var i = 0; i < sources.length; i++) {
    const source = sources[i];
  }
}

instead.

Fixed by #3165

sebmck reviewed Nov 5, 2015
View reviewed changes

Comment thread packages/babel-plugin-transform-es2015-parameters/src/rest.js Outdated

sebmck Nov 5, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optimized -> optimised.

jamiebuilds reviewed Nov 9, 2015
View reviewed changes

vhf mentioned this pull request Nov 10, 2015

Patterns as operands of the rest parameters #2965

Closed

vhf force-pushed the rest-arguments-use branch 2 times, most recently from 1fb31c9 to 1cd8cb7 Compare November 18, 2015 13:51

Have es2015 rest transform safely use arguments

306b5db

vhf force-pushed the rest-arguments-use branch from 1cd8cb7 to b82138e Compare November 19, 2015 11:48

vhf added 2 commits November 19, 2015 13:47

Factor out optimiseCandidate to reduce nesting & complexity

a510e73

T2965: Patterns as rest parameter operands are not valid, therefore t…

9a97d92

…his relic should go

vhf force-pushed the rest-arguments-use branch from b82138e to 9a97d92 Compare November 19, 2015 12:52

eventualbuddha reviewed Nov 19, 2015
View reviewed changes

DrewML mentioned this pull request Nov 22, 2015

Add some basic best practices jamiebuilds/babel-handbook#8

Merged

hzoo added the area: perf label Dec 10, 2015

sebmck added a commit that referenced this pull request Dec 11, 2015

Merge pull request #2833 from vhf/rest-arguments-use

e9fa841

Have es2015 rest transform safely use `arguments`

sebmck merged commit e9fa841 into babel:master Dec 11, 2015

kuraga reviewed Dec 14, 2015
View reviewed changes

This was referenced Dec 14, 2015

[optimisation] Simplify rest transformation #3164

Closed

[optimisation] Fixing T6818 #3165

Merged

amasad mentioned this pull request Dec 16, 2015

[babel-plugin-transform-es2015-parameters][T6774] more opportunities for optimizations #3143

Merged

vhf added a commit to vhf/babel that referenced this pull request Dec 21, 2015

Add test case for regression possibly introduced by babel#2833

f472670

Uh oh!

Uh oh!

Conversation

vhf commented Nov 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhf commented Nov 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 19, 2015

Current coverage is 89.20%

Uh oh!

vhf commented Nov 19, 2015

Uh oh!

jamiebuilds commented Nov 19, 2015

Uh oh!

sebmck commented Nov 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmck commented Dec 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chpio commented Dec 20, 2015

Uh oh!

vhf commented Dec 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Current coverage is `89.20%`