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

Hoist omitted keys from object spread operator #13384

Merged

Conversation

alanorozco
Copy link
Contributor

@alanorozco alanorozco commented May 27, 2021

Q                       A
Fixed Issues? N/A
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

The object ...spread operator will create a call to the objectWithoutProperties helper. The second argument for this helper is a list of omitted keys:

const { a, b, ...rest } = foo;
const { a, b } = foo;
const rest = babelHelpers.objectWithoutProperties(foo, ["a", "b"]);

When the destructuring statement for ...rest is inside a function, it's inneficient to create the array each time.

This is a common pattern in functional components, like on React:

function Component({ a, ...rest }) {}
function Component(_arg) {
  const { a } = _arg;
  const rest = babelHelpers.objectWithoutProperties(foo, ["a", "b"]);
}

Now hoisted to the module scope:

const _temp = ["a", "b"];

function Component(_arg) {
  const { a } = _arg;
  const rest = babelHelpers.objectWithoutProperties(foo, _temp);
}

@babel-bot
Copy link
Collaborator

babel-bot commented May 27, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 27, 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 c01a0e0:

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

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

There's also a similar transform happening in packages/babel-plugin-transform-destructuring/src/index.js which should be updated.


A little more context:

We use Closure Compiler to minify our code, and one of the optimizations it to turn long string arrays into a "foo bar".split(" "). When this is only done once, it's not much of a problem, but when it's run multiple times in a function, it becomes a huge performance penalty. Take an abridged (real world) example:

// input
function Component(_ref) {
  const {
    as,
    children,
    className,
    contentAs,
    contentClassName,
    contentProps,
    contentRef,
    contentStyle,
    layout,
    paint,
    size,
    style,
    wrapperClassName,
    wrapperStyle
  } = _ref;
  const rest = objectWithoutProperties(_ref, ["as", "children", "className", "contentAs", "contentClassName", "contentProps", "contentRef", "contentStyle", "layout", "paint", "size", "style", "wrapperClassName", "wrapperStyle"]);
  return rest;
}


// Output
function Component(a){return objectWithoutProperties(a,"as children className contentAs contentClassName contentProps contentRef contentStyle layout paint size style wrapperClassName wrapperStyle".split(" "))};

We know as Babel devs that the array here is constant and won't be mutated by objectWithoutProperties. But, Closure doesn't, and it can't hoist the array itself. Because of that, we end up with the inefficient split(" ") causing slowdowns in our code.

By hoisting this array in Babel, Closure can still generate a minified string split, but it'll only be evaluated once now. And, we can receive the best of size and performance.

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.

As a further optimization, we could:

  • sort the exclude keys (so that two destructurings with at least partially overlapping excluded keys can benefit more from gzip)
  • If there are two destructurings in the same file excluding the same keys, we can re-use the same _exclude variable

However, we would need some real-world data to prove that it's worth it.

@nicolo-ribaudo nicolo-ribaudo added the PR: Output optimization 🔬 A type of pull request used for our changelog categories label May 28, 2021
@nicolo-ribaudo
Copy link
Member

Can you run OVERWRITE=true make test or OVERWRITE=true yarn jest?

@nicolo-ribaudo nicolo-ribaudo merged commit f35513f into babel:main May 28, 2021
@alanorozco alanorozco deleted the hoist-object-spread-omitted-keys branch May 28, 2021 20:12
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2021
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 PR: Output optimization 🔬 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