Skip to content

[optimisation] Fixing T6818 - #3165

Merged
amasad merged 5 commits into
babel:masterfrom
vhf:multiple-rest-arguments
Dec 21, 2015
Merged

[optimisation] Fixing T6818#3165
amasad merged 5 commits into
babel:masterfrom
vhf:multiple-rest-arguments

Conversation

@vhf

@vhf vhf commented Dec 14, 2015

Copy link
Copy Markdown
Contributor

#2833 : applying the same transform to multiple params ending with rest.

Without this PR:

var t = function (...items) {
    var x = items[0];
    var y = items[1];
}
// ==>
var t = function () {
    var x = arguments.length <= 0 ? undefined : arguments[0];
    var y = arguments.length <= 1 ? undefined : arguments[1];
};
// BUT:
function u(f, g, ...items) {
    var x = f;
    var y = g;
    x = items[0];
    y = items[1];
}
// ==>
function u(f, g) {
    var x = f;
    var y = g;
    x = arguments[2];
    y = arguments[3];
}

After this PR:

function u(f, g, ...items) {
    var x = f;
    var y = g;
    x[12] = items[0];
    y.prop = items[1];
    var z = items[2] | 0 || 12;
}
// ==>
function u(f, g) {
    var x = f;
    var y = g;
    x[12] = arguments.length <= 2 ? undefined : arguments[2];
    y.prop = arguments.length <= 3 ? undefined : arguments[3];
    var z = (arguments.length <= 4 ? undefined : arguments[4]) | 0 || 12;
}

Usual biased benchmark: https://gist.github.com/vhf/804cbddf89e4c8c8d8e2

before x 19,363 ops/sec ±0.94% (86 runs sampled)
 after x 78,760 ops/sec ±0.72% (97 runs sampled)

@codecov-io

codecov-io commented Dec 14, 2015

Copy link
Copy Markdown

Current coverage is 84.77%

Merging #3165 into master will decrease coverage by -0.27% as of f472670

@@            master   #3165   diff @@
======================================
  Files          215     215       
  Stmts        15623   15617     -6
  Branches      3338    3337     -1
  Methods          0       0       
======================================
- Hit          13286   13239    -47
- Partial        684     728    +44
+ Missed        1653    1650     -3

Review entire Coverage Diff as of f472670

Powered by Codecov. Updated on successful CI builds.

@vhf vhf changed the title [optimisation] Safely transform multiple rest arguments [optimisation] Fixing T6818 Dec 14, 2015
amasad
amasad reviewed Dec 14, 2015
View reviewed changes
Comment thread packages/babel-plugin-transform-es2015-parameters/src/rest.js Outdated

@amasad amasad Dec 14, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is left referring to here? Also note that you already have access to parent no need for parentPath.parent

@vhf vhf Dec 16, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the review, I really appreciate it!

Here, left was referring to the left-hand side of an assignment operation. It was way too complicated. Covering all cases in this condition is an impossible task.

I don't understand your second comment about parent and parentPath. I need parent to get to the properties and parentPath to replaceWith. Is there a workaround?

@amasad

amasad commented Dec 16, 2015

Copy link
Copy Markdown
Member

LGTM. A lot cleaner and less code. I'll let it sit for a bit more if @sebmck wants to take a look.

@amasad amasad mentioned this pull request Dec 16, 2015
Merged
@jamiebuilds

jamiebuilds commented Dec 17, 2015

Copy link
Copy Markdown
Contributor

Nice work

@vhf
vhf force-pushed the multiple-rest-arguments branch from 15e7d73 to 6cc0538 Compare December 18, 2015 10:31
@vhf

vhf commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews @amasad & @thejameskyle. Merging #3143 introduced conflicts, I rebased my patch on top of the current master.

@hzoo

hzoo commented Dec 18, 2015

Copy link
Copy Markdown
Member

Looks like a test is failing now (added from the other PR)

@vhf

vhf commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

Oops, yep. Seems like #3143 changes are not compatible with mine, although it merged seamlessly. I'll see what I can do.

@vhf

vhf commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

Ok @hzoo, I pushed a fix.

While I'm quite certain this fix is enough, I'm not sure if (and how) we could refactor this thing. Having both deopt and replaceOnly flags is not very nice. But perhaps it's still better than having three separate flags loopOpt, argsOpt and replaceOpt in state.

At the moment, this transform optimises several different cases (f(...items)):

  1. items[0] -> safe access to arguments
  2. items.length -> arguments.length
  3. g(...items) call -> g.apply
  4. else reconstruct items by looping over arguments

I'm open to suggestions.

@vhf vhf mentioned this pull request Dec 20, 2015
Merged
@amasad

amasad commented Dec 21, 2015

Copy link
Copy Markdown
Member

I think that's fine

amasad added a commit that referenced this pull request Dec 21, 2015
@amasad
Merge pull request #3165 from vhf/multiple-rest-arguments
6354e71 
[optimisation] Fixing T6818
@amasad
amasad merged commit 6354e71 into babel:master Dec 21, 2015
@jmm jmm mentioned this pull request Jan 7, 2016
Merged
@lock lock Bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock Bot locked as resolved and limited conversation to collaborators Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vhf @codecov-io @amasad @jamiebuilds @hzoo