[Babel 8] Improve scope information collection performance #17043
Conversation
| resync.call(path); | ||
|
|
||
| // this path no longer belongs to the tree | ||
| if (path.key === null) continue; |
There was a problem hiding this comment.
This is a new change, which avoids unnecessary scope information collection.
There was a problem hiding this comment.
It seems to me this change can be shipped to Babel 7, right?
There was a problem hiding this comment.
Yes. In fact the whole PR can now be shipped to Babel 7, there are no known incompatibilities (even with babel-minify).
We are just postponing it to Babel 8 to be conservative.
There was a problem hiding this comment.
I also moved it to Babel 8 for safety.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59979 |
liuxingbaoyu
force-pushed
the
traverseForScope2
branch
from
3c1c980 to
51eda74
Compare
liuxingbaoyu
force-pushed
the
traverseForScope2
branch
from
51eda74 to
0821127
Compare
liuxingbaoyu
force-pushed
the
traverseForScope2
branch
from
0821127 to
ac26b44
Compare
liuxingbaoyu
force-pushed
the
traverseForScope2
branch
from
ac26b44 to
99412f1
Compare
|
commit: |
|
Do you have a before/after benchmark? |
|
Oh nevermind, it's in #16923 |
| this._traverseFlags |= SHOULD_SKIP | SHOULD_STOP; | ||
| } | ||
|
|
||
| export function _forceSetScope(current: NodePath) { |
There was a problem hiding this comment.
Does this end up being exposed on NodePath.prototype? Maybe lets just move it to where we use it.
There was a problem hiding this comment.
Yes, it's private.
If we need to change setScope later, keeping them together helps us not forget to change _forceSetScope. :)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // Skip method scope if is computed method key or decorator expression | ||
| ((current.key === "key" || current.listKey === "decorators") && | ||
| path.isMethod()) || |
There was a problem hiding this comment.
Could you add tests covering this change? It seems to me that tests are passing without this change.
There was a problem hiding this comment.
Great find!
They were copied from setScope and are useless on this path, so I'll remove them.
I'll try to add a test.
liuxingbaoyu
force-pushed
the
traverseForScope2
branch
from
aaec1a6 to
722009e
Compare
| it.each([ | ||
| `switch (a) { default: let a = "inside" }`, | ||
| `class foo { @a m() { var a = "inside"; } }`, | ||
| `class foo { [a]() { var a = "inside" } }`, | ||
| ])("scope hasGlobal with crawl: `%s`", function (code) { | ||
| let hasGlobal; | ||
| traverse( | ||
| parse(code, { | ||
| plugins: [["decorators", { version: "2025-03" }]], | ||
| }), | ||
| { | ||
| Identifier(path) { | ||
| if ( | ||
| path.node.name === "a" && | ||
| path.parent.type !== "VariableDeclarator" && | ||
| path.parent.type !== "AssignmentPattern" | ||
| ) { | ||
| hasGlobal = path.scope.hasGlobal("a"); | ||
| } | ||
| }, | ||
| }, | ||
| ); | ||
| expect(hasGlobal).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The tests here are indeed weird, but they are the only behavioral changes I could find.
JLHwung
added
the
PR: Performance (next major) 🏃♀️
| current.scope?.init(); | ||
| } | ||
|
|
||
| export function setScope(this: NodePath) { |
There was a problem hiding this comment.
Can we make setScope just call _forceSetScope(this)?
There was a problem hiding this comment.
Wow, awesome find!
Thanks to #16965, cache pollution issues are gone in Babel 8, and we can safely remove them.
| if (process.env.BABEL_8_BREAKING && path.opts?.noScope) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is a new improvement, I realized that we don't need to collect scope information when noScope.
4 participants
Fixes #1, Fixes #2Ref: #17005