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

getBindingIdentifiers should return params for private methods #13723

Merged
merged 6 commits into from Sep 2, 2021

Conversation

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 1, 2021

Q                       A
Fixed Issues? t.getBindingIdentifiers does not return parameter bindings for private class methods
Patch: Bug Fix? Y
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Also include minor tweaks around scope tracking.

Performant issue

The getBindingIdentifiers is generally slow due to heavy dynamics: it even allows monkey-patching its visitor keys: getBindingIdentifiers.keys can be modified by external plugins. In my local perf test, dropping getBindingIdentifiers.keys can achieves 10x performance boost:

baseline 16 binding identifiers in patterns: 61_364 ops/sec ±0.78% (0.016ms)
baseline 32 binding identifiers in patterns: 27_504 ops/sec ±1.76% (0.036ms)
baseline 64 binding identifiers in patterns: 11_901 ops/sec ±0.75% (0.084ms)
baseline 128 binding identifiers in patterns: 4_260 ops/sec ±1.49% (0.235ms)
current 16 binding identifiers in patterns: 422_790 ops/sec ±1.03% (0.002ms)
current 32 binding identifiers in patterns: 193_291 ops/sec ±1.5% (0.005ms)
current 64 binding identifiers in patterns: 75_164 ops/sec ±1.45% (0.013ms)
current 128 binding identifiers in patterns: 27_211 ops/sec ±1.04% (0.037ms)
current2 16 binding identifiers in patterns: 432_890 ops/sec ±1.55% (0.002ms)
current2 32 binding identifiers in patterns: 250_765 ops/sec ±0.94% (0.004ms)
current2 64 binding identifiers in patterns: 118_529 ops/sec ±1.45% (0.008ms)
current2 128 binding identifiers in patterns: 58_682 ops/sec ±1.88% (0.017ms)

current is new implementation inlining visitor keys, current2 is a new implementation based on current which returns bindings in tail-first order: so we can avoid unnecessary memcpy introduced by array.shift in

we should reconsider whether we are going to support getBindingIdentifiers.keys in Babel 8.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Sep 1, 2021

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

@codesandbox
Copy link

@codesandbox codesandbox bot commented Sep 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5bfc350:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

hzoo
hzoo approved these changes Sep 2, 2021
@@ -5,7 +5,6 @@ import type { TraverseOptions } from "../index";
import Binding from "./binding";
import globals from "globals";
import {
FOR_INIT_KEYS,
Copy link
Member

@hzoo hzoo Sep 2, 2021

nice refactor here!

Copy link
Contributor Author

@JLHwung JLHwung Sep 2, 2021

The FOR_INIT_KEYS is only used here. After this PR we may even remove it in Babel 8 if nobody else is using.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

👍

@JLHwung JLHwung force-pushed the fix-get-binding-identifiers branch from 5ab190d to 5bfc350 Sep 2, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit 51c6a99 into babel:main Sep 2, 2021
26 of 28 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the fix-get-binding-identifiers branch Sep 2, 2021
@nicolo-ribaudo nicolo-ribaudo changed the title getBindingIdentifiers should return params for private methods getBindingIdentifiers should return params for private methods Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants