Skip to content
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

Traverse performance #10480

Merged
merged 8 commits into from Nov 5, 2019
Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 22, 2019

Q                       A
Minor: Bug Fix? 👍
Major: Breaking Change? No if one is using public API.
Tests Added + Pass? Yes
License MIT

In this PR we improve the performance of babel-traverse by revising the memory layout of NodePath. It results to 10% performance gain compared to the control branch on the following test cases.

const babel = require("/path/to/local/babel-core");
const code = require("fs").readFileSync("./fixtures/es5/babylon-dist.js");
babel.transform(code, { preset: ["@babel/env"] });

I can observe this branch outperforms control branch consistently.

┌─────────────────────┬──────────────────────────────┐
│ fixture             │ devTransform                 │
├─────────────────────┼──────────────────────────────┤
│ es5/babylon-dist.js │ 1.02 ops/sec ±31.84% (978ms) │
└─────────────────────┴──────────────────────────────┘
┌─────────────────────┬───────────────────────────────┐
│ fixture             │ baseTransform                 │
├─────────────────────┼───────────────────────────────┤
│ es5/babylon-dist.js │ 0.86 ops/sec ±20.52% (1167ms) │
└─────────────────────┴───────────────────────────────┘

And the memory usage improvements due to compressing traverse flags and adding accessor methods for inList and parentPath.

┌─────────────────────┬──────────────────┐
│ fixture             │ devTransform x5  │
├─────────────────────┼──────────────────┤
│ es5/babylon-dist.js │ heap: 239.37 MiB │
└─────────────────────┴──────────────────┘
┌─────────────────────┬──────────────────┐
│ fixture             │ baseTransform x5 │
├─────────────────────┼──────────────────┤
│ es5/babylon-dist.js │ heap: 305.47 MiB │
└─────────────────────┴──────────────────┘

The following changes are implemented:

  • Removed NodePath.inList data property, a get accessor is provided for backward compatibility (partially).
  • Removed NodePath.parentKey data property, a get accessor is provided for backward compatibility (partially).
  • Removed NodePath.typeAnnotation data property, it is introduced in Initialize properties to avoid hidden class thrashing. #1752 but since then it is not used by any routines. I believe it is a mistype of Node.typeAnnotation.
  • Compressed NodePath.shouldSkip/shouldStop/removed into _traverseFlags bit array, both get accessor and set accessor is provided for backward compatibility.
  • Lazily assign this.skipKeys in setContext.
  • Lazily initialize this.data in NodePath.constructor.

In practice inList and parentKey should be treated as read-only since these two properties are rewritten in setContext. The current behavior accepts one assigning these two properties.

The shouldSkip/shouldStop/removed traverse flags are handy and turbofan would inline these simple accessors we don't introduce performance overhead here. By compressing these states into bit array we optimize the performance of setContext.

@JLHwung JLHwung added pkg: traverse PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories labels Sep 22, 2019
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Regarding the breaking changes, I think that any plugin relying on the old behavior was actually broken: if someone wanted to change the parent node by doing path.parentPath = ... it wouldn't have worked.

packages/babel-traverse/src/index.js Show resolved Hide resolved
packages/babel-traverse/src/index.js Outdated Show resolved Hide resolved
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 22, 2019

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

@@ -92,12 +93,14 @@ export function skip() {
}

export function skipKey(key) {
if (this.skipKeys == null) {
this.skipKeys = {};
}
Copy link
Member

Choose a reason for hiding this comment

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

I wish https://github.com/tc39/proposal-logical-assignment wasn't just stage 1 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offtopic: we still need a PR to add nullish coalescing and optional chaining.

Copy link
Member

@hzoo hzoo Sep 24, 2019

Choose a reason for hiding this comment

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

We can always use new stage features if you are willing to move off of them if they change (I think that one is small enough that it's fine) 😁

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 22, 2019

@nicolo-ribaudo I am sorry I make a typo before. It should be the parentKey instead of parentPath, but your point is still valid on parentKey.

I think that any plugin relying on the old behavior was actually broken: if someone wanted to change the parent node by doing path.parentKey = ... it wouldn't have worked.

I agree with you that setting inList and/or parentKey never works in practice. Instead of failing silently, babel will now report can not assign to an accessor. In this way it seems more like a bug fix, though. 😁

@buildsize
Copy link

buildsize bot commented Sep 24, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.69 MB 2.74 MB 52.46 KB (2%)
babel-preset-env.min.js 1.63 MB 1.65 MB 24.77 KB (1%)
babel.js 2.9 MB 2.95 MB 52.47 KB (2%)
babel.min.js 1.6 MB 1.63 MB 24.78 KB (2%)

@nicolo-ribaudo nicolo-ribaudo added this to the v7.7.0 milestone Sep 24, 2019
Comment on lines +26 to +28
export const REMOVED = 1 << 0;
export const SHOULD_STOP = 1 << 1;
export const SHOULD_SKIP = 1 << 2;
Copy link
Member

Choose a reason for hiding this comment

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

Some great work doing this research!

I would just like to note that using bitwise flags and operators is for sure going to make contributing and reading the coding that much more difficult (for me, and I will assume some others). I think I mentioned it earlier on Slack and it's a hard call but trading off readability/performance. I don't know what else we can do other than writing comments to explain things though

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Needs Review A pull request awaiting more approvals PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants