Skip to content

Comments

Use single object spread call in loose mode#11520

Merged
nicolo-ribaudo merged 3 commits intobabel:masterfrom
jridgewell:loose-object-spread
May 8, 2020
Merged

Use single object spread call in loose mode#11520
nicolo-ribaudo merged 3 commits intobabel:masterfrom
jridgewell:loose-object-spread

Conversation

@jridgewell
Copy link
Member

Q                       A
Fixed Issues? #11471 (comment)
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Does 2 things:

  • Makes every exec.js test run in a new VM context (necessary for the new tests)
  • Reuses the same object spread call in loose mode, making the output considerably smaller.
// Old
var output = _extends(_extends(_extends(_extends(_extends(_extends({}, pureA), {}, {
  get foo() {},
  get bar() {}
}, pureB), pureC), impureFunc()), pureD), {}, {
  pureD
});

// New
var output = _extends({}, pureA, {
  get foo() {},
  get bar() {}
}, pureB, pureC, impureFunc(), pureD, {
  pureD
});

@@ -0,0 +1,43 @@
"use strict";
Object.defineProperty(Object.prototype, 'NOSET', {
Copy link
Member Author

Choose a reason for hiding this comment

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

These were polluting the shared context, causing the other tests to fail.

@babel-bot
Copy link
Collaborator

babel-bot commented May 4, 2020

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 4, 2020

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 654e0a3:

Sandbox Source
hungry-shadow-01m29 Configuration
relaxed-sound-gnfh6 Configuration

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Comparing the CI build time between master and this PR, the unit test now costs 30% longer time. My gut is it comes from the cost of creating every single context and evaluating @babel/polyfill and babel-helpers-in-memory.

Instead of running common modules for every single exec test, can we leverage the cacheData option? We can generate code cache after running common modules, and supply it to every single exec test context.

@nicolo-ribaudo nicolo-ribaudo added PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories Spec: Object Rest/Spread labels May 5, 2020
@jridgewell
Copy link
Member Author

jridgewell commented May 6, 2020

Instead of running common modules for every single exec test, can we leverage the cacheData option? We can generate code cache after running common modules, and supply it to every single exec test context.

Excellent idea! Done. This gave me a roughly 20% speedup.

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Makes every exec.js test run in a new VM context (necessary for the new tests)

Can we separate this into a new PR? After the code cache, running yarn jest object-rest-spread now costs 8 secs vs. 4 sec on master.

Note that we can circumvent this issue by adding some suffix.

Object.defineProperty(Object.prototype, `NOSET${__filename}`, {
  get(value) {
    // noop
  },
});

I am good on the loose mode changes, but we should see if we can do better on the exec context.

@jridgewell jridgewell force-pushed the loose-object-spread branch from d1fcc8f to 654e0a3 Compare May 7, 2020 03:31
@jridgewell
Copy link
Member Author

Can we separate this into a new PR?

Done.

@jridgewell jridgewell mentioned this pull request May 7, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit a080a1d into babel:master May 8, 2020
@jridgewell jridgewell deleted the loose-object-spread branch May 8, 2020 16:32
@cpojer
Copy link
Member

cpojer commented May 10, 2020

Thank you for making this change, excited to try it out once it is released :)

@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 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 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 PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories Spec: Object Rest/Spread

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants