Merge internal transformers into single traversal pass by sebmck · Pull Request #1472 · babel/babel

sebmck · 2015-05-07T14:56:38Z

Complicates the internal transformer state fairly signifcantly but adds a transformer metadata group property that allows the merging of transformer visitors with the same group.

This isn't ready to be merged yet. Currently around ~100 tests failing. These are a combination of mixed state issues and infinite recursion bugs due to the changes required in the traversal algorithm to accommodate multi-method visitors.

cc @amasad @DmitrySoshnikov

sebmck · 2015-05-07T14:57:31Z

Whitespace and comments were removed because the comments were outdated and the whitespace separating transformers was pretty arbitrary and gave the impression that it was somehow significant/related when it wasn't in the slightest.

…xit rather than on entrance

sebmck · 2015-05-07T15:32:34Z

Before

$ DEBUG=babel time babel tsserver.js --out-file tsserver.foo.js
  babel [BABEL] tsserver.js: Parse start +0ms
  babel [BABEL] tsserver.js: Parse stop +752ms
  babel [BABEL] tsserver.js: Start set AST +2ms
  babel [BABEL] tsserver.js: Start scope building +0ms
  babel [BABEL] tsserver.js: End scope building +3s
  babel [BABEL] tsserver.js: End set AST +893ms
  babel [BABEL] tsserver.js: Start prepass +0ms
  babel [BABEL] tsserver.js: End prepass +2s
  babel [BABEL] tsserver.js: Start module formatter init +0ms
  babel [BABEL] tsserver.js: End module formatter init +1s
  babel [BABEL] tsserver.js: Start transformer strict +0ms
  babel [BABEL] tsserver.js: Finish transformer strict +4ms
  babel [BABEL] tsserver.js: Start transformer _validation +0ms
  babel [BABEL] tsserver.js: Finish transformer _validation +698ms
  babel [BABEL] tsserver.js: Start transformer validation.react +0ms
  babel [BABEL] tsserver.js: Finish transformer validation.react +698ms
  babel [BABEL] tsserver.js: Start transformer spec.functionName +0ms
  babel [BABEL] tsserver.js: Finish transformer spec.functionName +708ms
  babel [BABEL] tsserver.js: Start transformer spec.blockScopedFunctions +0ms
  babel [BABEL] tsserver.js: Finish transformer spec.blockScopedFunctions +721ms
  babel [BABEL] tsserver.js: Start transformer es6.tailCall +0ms
  babel [BABEL] tsserver.js: Finish transformer es6.tailCall +1s
  babel [BABEL] tsserver.js: Start transformer _blockHoist +0ms
  babel [BABEL] tsserver.js: Finish transformer _blockHoist +699ms
  babel [BABEL] tsserver.js: Start transformer _shadowFunctions +0ms
  babel [BABEL] tsserver.js: Finish transformer _shadowFunctions +2s
  babel [BABEL] tsserver.js: Start transformer _strict +0ms
  babel [BABEL] tsserver.js: Finish transformer _strict +1ms
  babel [BABEL] tsserver.js: Start transformer _moduleFormatter +0ms
  babel [BABEL] tsserver.js: Finish transformer _moduleFormatter +0ms
  babel [BABEL] tsserver.js: Start transformer es3.propertyLiterals +0ms
  babel [BABEL] tsserver.js: Finish transformer es3.propertyLiterals +712ms
  babel [BABEL] tsserver.js: Start transformer es3.memberExpressionLiterals +0ms
  babel [BABEL] tsserver.js: Finish transformer es3.memberExpressionLiterals +744ms
  babel [BABEL] tsserver.js: Generation start +1ms
[BABEL] Note: The code generator has deoptimised the styling of "tsserver.js" as it exceeds the max of "100KB".
  babel [BABEL] tsserver.js: Generation end +2s
       18.10 real        18.34 user         0.27 sys

After

$ DEBUG=babel time babel tsserver.js --out-file tsserver.foo.js
  babel [BABEL] tsserver.js: Parse start +0ms
  babel [BABEL] tsserver.js: Parse stop +752ms
  babel [BABEL] tsserver.js: Start set AST +1ms
  babel [BABEL] tsserver.js: Start scope building +1ms
  babel [BABEL] tsserver.js: End scope building +3s
  babel [BABEL] tsserver.js: End set AST +904ms
  babel [BABEL] tsserver.js: Start module formatter init +0ms
  babel [BABEL] tsserver.js: End module formatter init +1s
  babel [BABEL] tsserver.js: Start transformer builtin +1ms
  babel [BABEL] tsserver.js: Finish transformer builtin +6s
  babel [BABEL] tsserver.js: Generation start +1ms
[BABEL] Note: The code generator has deoptimised the styling of "tsserver.js" as it exceeds the max of "100KB".
  babel [BABEL] tsserver.js: Generation end +2s
       15.07 real        15.32 user         0.27 sys

Not nearly as substantial as I'd hoped 😞

sebmck · 2015-05-07T17:06:48Z

Actually, seems to make some difference. Compiling the entire Ember core source with Babel on master takes:

real    0m50.575s
user    0m51.257s
sys 0m0.478s

On this branch it takes:

real    0m35.426s
user    0m36.004s
sys 0m0.580s

Going to need to get some more numbers before I move forward with finishing this refactor though since I don't want to waste too much time if it's pointless.

amasad · 2015-05-07T17:16:20Z

💃
Trying to run it. Running into this issue:

class bar extends wow {
    constructor() {
        super();
        var x =() => this.wat();
    }
}

/Users/amasad/code/babel/lib/babel/transformation/file/index.js:616
      throw err;
            ^
TypeError: unknown: Cannot read property 'start' of undefined
    at TraversalPath.errorWithNode (/Users/amasad/code/babel/lib/babel/traversal/path/index.js:478:28)
    at TraversalPath.enter (/Users/amasad/code/babel/lib/babel/transformation/transformers/es6/classes.js:136:20)
    at TraversalPath.call (/Users/amasad/code/babel/lib/babel/traversal/path/index.js:750:28)
    at TraversalPath.visit (/Users/amasad/code/babel/lib/babel/traversal/path/index.js:781:10)
    at TraversalContext.visitSingle (/Users/amasad/code/babel/lib/babel/traversal/context.js:72:41)
    at TraversalContext.visit (/Users/amasad/code/babel/lib/babel/traversal/context.js:82:19)
    at Function.traverse.node (/Users/amasad/code/babel/lib/babel/traversal/index.js:62:17)
    at TraversalPath.visit (/Users/amasad/code/babel/lib/babel/traversal/path/index.js:798:31)
    at TraversalContext.visitMultiple (/Users/amasad/code/babel/lib/babel/traversal/context.js:59:16)
    at TraversalContext.visit (/Users/amasad/code/babel/lib/babel/traversal/context.js:80:19)

sebmck · 2015-05-07T17:23:38Z

Grr, looks like it's due to the es6.arrowFunctions transformer. Can you try blacklisting it?

amasad · 2015-05-07T17:24:28Z

works!

sebmck · 2015-05-07T17:25:25Z

Yay!

sebmck · 2015-05-07T17:26:53Z

Compiling Traceur which has a ton of ES6 so it's a more reflective benchmark:

master

real    0m30.877s
user    0m31.675s
sys 0m0.290s

single-pass

real    0m20.517s
user    0m21.308s
sys 0m0.297s

sebmck · 2015-05-07T17:27:23Z

Seeing on average a ~30% improvement on actual ES6 codebases.

amasad · 2015-05-07T17:38:07Z

A 130-150 seconds build is now 80-90 seconds.

I wish I had your emoji game right now, but here is my best:

💥 👊 😝 👯 👯

amasad · 2015-05-07T17:40:41Z

extra newlines

DmitrySoshnikov · 2015-05-07T17:45:06Z

This is a fantastic PR guys! 😄 👍

amasad · 2015-05-07T17:56:19Z

amasad · 2015-05-07T17:59:29Z

So shouldVisit is deprecated in favor of category?

sebmck · 2015-05-07T18:02:35Z

Moreorless, yeah. Doing the prepass itself was pretty expensive, especially on large trees and the "merged" visitors basically eliminates it's usecase.

amasad · 2015-05-07T18:06:53Z

I don't see any category for builtin transforms, are they all in a single pass now? Otherwise on what basis is the merge decided?

sebmck · 2015-05-07T18:07:40Z

I kinda cheated a bit and add the category here.

amasad · 2015-05-07T18:12:07Z

I'm surprised they all can now work in a single pass. How did you get rid of ordering?
For example in the comments in src/babel/transformation/transformers/index.js it used to say which transform needs to run first.

sebmck · 2015-05-07T18:18:45Z

Well they sorta work in a single pass. There's still 97/414 tests failing.

Basically, previously the visitor format was:

{
  Identifier: {
    enter: Function,
    exit: Function
  }
}

Now enter and exit can be arrays:

{
  Identifier: {
    enter: [Function],
    exit: [Function]
  }
}

The mergeStack method just crawls all the enabled transformers, and when it hits a transformer with a category collects all the proceeding transformers with the same category, merges their visitors and creates a new transformer in their place.

Ordering wasn't really removed. If you have two transformers that define Identifier.enter methods then the order in which those methods are ran in is determined by the "global position" (ie. just what position the transformer is in that transformers object). It causes some weird issues where enter methods need to be turned to exit ones and that's largely been what most of the changes have been.

amasad · 2015-05-08T14:38:34Z

Ok, can you check in (or create another repo) with your Traceur benchmark? so we can track how we're doing

sebmck · 2015-05-08T14:55:49Z

Sure, there's a single syntax error that Traceur allows that Babel doesn't that I've had to change but just checkout this fork and run:

$ time babel src --out-dir lib --blacklist es6.destructuring

There are some oddities with the es6.destructuring transformer that are yet to be fixed which is why it's blacklisted.

Another thing I've found is that blacklisting regenerator shaves a few seconds off, this is likely since ast-types is doing it's own scope tracking.

… calling pre and post methods

…rder of what they need and what they actually do

…lasses transformer

…loops in the es6.blockScoping transformer

Merge internal transformers into single traversal pass

sebmck · 2015-05-08T22:36:46Z

All tests now pass. Going to merge into master before I break something again.

amasad · 2015-05-08T22:37:20Z

Boom! Any final numbers?

sebmck · 2015-05-08T22:45:50Z

Traceur:

Before:

real    0m30.877s
user    0m31.675s
sys 0m0.290s

After:

real    0m24.015s
user    0m24.907s
sys 0m0.274s

Ember:

Before:

real    0m50.575s
user    0m51.257s
sys 0m0.478s

After:

real    0m43.535s
user    0m44.160s
sys 0m0.485s

Not as dramatic as I would've hoped 😞 Need to find out where that extra bit went. Mind running it on one of your apps?

amasad · 2015-05-08T22:50:12Z

Will do

amasad · 2015-05-08T23:26:29Z

It's a consistent 20% This is without adding category to our plugins. Good job, man! 🍶

sebmck · 2015-05-08T23:28:09Z

Awesome! Also I renamed the category option to group since it's more descriptive.

stefanpenner · 2015-05-08T23:33:26Z

👍

benjamn · 2015-05-09T00:16:52Z

Sweet!

merge internal transformers into single traversal pass

6f664ca

sebmck reviewed May 7, 2015
View reviewed changes

sebmck added 3 commits May 7, 2015 16:01

don't requeue node unless the type has changed

9ffc265

do member expression and property literal conversion to computed on e…

f17b268

…xit rather than on entrance

do module formatter on program exit rather than entrance

0bbfd42

amasad reviewed May 7, 2015
View reviewed changes

Comment thread src/babel/transformation/file/index.js

Copy link
Copy Markdown

Member

amasad May 7, 2015

Choose a reason for hiding this comment

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

👍

sebmck added 5 commits May 7, 2015 20:11

attach auxiliary comment to function declaration helpers - fixes #1476

a1bc070

add Super node type to ast-types patch

23ac231

Merge branch 'master' into single-pass

0b15a97

add transformer dependencies - fixes #1477

380293d

add node existence check to each visitor call

13a6c69

sebmck added 2 commits May 8, 2015 15:51

move shadow-functions internal transformer to builtin-cleanup category

c526a3e

update flow strip type annotations test

4a439d8

sebmck added 12 commits May 8, 2015 22:51

don't pretty print variable declarators in concise mode

abf4a54

remove incomplete ludicrous transformer

2a9a59f

update classes to reflect new transformer precedence

d6a5c39

fix replacement requeue exiting too early

8292d6d

store uncollapsed transformer pass stack so we have access to it when…

b376b6b

… calling pre and post methods

further work splitting up transformers into their own "category" in o…

8f9d02d

…rder of what they need and what they actually do

rename category metadata option to group

2fd1b2a

move hoistDirectives transformer to builtin-setup

590a165

change ClassExpression visitor to enter instead of exist in the es6.c…

b02a111

…lasses transformer

skip traversing back into ReturnStatement replacement when rewriting …

f0bfdfb

…loops in the es6.blockScoping transformer

move shadow function visitors to entrance instead of exit

920c84a

ensure dynamic imports are hoisted

89e3108

sebmck added a commit that referenced this pull request May 8, 2015

Merge pull request #1472 from babel/single-pass

61ba8ad

Merge internal transformers into single traversal pass

sebmck merged commit 61ba8ad into master May 8, 2015

sebmck deleted the single-pass branch May 8, 2015 22:36

sebmck mentioned this pull request May 9, 2015

Speeeeed #1486

Closed

13 tasks

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 8, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 8, 2019

Uh oh!

Conversation

sebmck commented May 7, 2015

Uh oh!

sebmck May 7, 2015

Choose a reason for hiding this comment

Uh oh!

sebmck commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

amasad commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

amasad commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

amasad commented May 7, 2015

Uh oh!

amasad May 7, 2015

Choose a reason for hiding this comment

Uh oh!

sebmck May 7, 2015

Choose a reason for hiding this comment

Uh oh!

DmitrySoshnikov commented May 7, 2015

Uh oh!

amasad May 7, 2015

Choose a reason for hiding this comment

Uh oh!

amasad commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

amasad commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

amasad commented May 7, 2015

Uh oh!

sebmck commented May 7, 2015

Uh oh!

amasad commented May 8, 2015

Uh oh!

sebmck commented May 8, 2015

Uh oh!

sebmck commented May 8, 2015

Uh oh!

amasad commented May 8, 2015

Uh oh!

sebmck commented May 8, 2015

Uh oh!

amasad commented May 8, 2015

Uh oh!

amasad commented May 8, 2015

Uh oh!

sebmck commented May 8, 2015

Uh oh!

stefanpenner commented May 8, 2015

Uh oh!

benjamn commented May 9, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants