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.

Sign up for GitHub

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

Jump to bottom

lib: switch to object spread where possible #25104

Closed
wants to merge 1 commit into from
Closed

lib: switch to object spread where possible #25104

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

Use the object spread notation instead of using Object.assign.
It is not only easier to read it is also faster as of V8 6.8.

Benchmark with 11.4:

// node benchmark/es/spread-assign.js 
es/spread-assign.js n=1000000 count=5 method="spread": 36,495,416.66835458
es/spread-assign.js n=1000000 count=10 method="spread": 31,889,730.923060697
es/spread-assign.js n=1000000 count=20 method="spread": 151,930.52448171392
es/spread-assign.js n=1000000 count=5 method="assign": 9,476,496.804852217
es/spread-assign.js n=1000000 count=10 method="assign": 4,704,920.0252048215
es/spread-assign.js n=1000000 count=20 method="assign": 123,309.56193177585
es/spread-assign.js n=1000000 count=5 method="_extend": 5,076,862.324845331
es/spread-assign.js n=1000000 count=10 method="_extend": 2,359,288.3170930305
es/spread-assign.js n=1000000 count=20 method="_extend": 382,746.2281006062
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR
lib: switch to object spread where possible
6b7fd0a 
Use the object spread notation instead of using Object.assign.
It is not only easier to read it is also faster as of V8 6.8.
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1988/pipeline

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 18, 2018
devsnek
devsnek previously approved these changes Dec 18, 2018
View reviewed changes
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 18, 2018 •
edited by Trott
Loading

CI https://ci.nodejs.org/job/node-test-pull-request/19639/ ✔️

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 18, 2018
@devsnek devsnek dismissed their stale review December 18, 2018 02:33

found a point we should address

devsnek
devsnek reviewed Dec 18, 2018
View reviewed changes
lib/child_process.js
@@ -136,7 +136,7 @@ function normalizeExecArgs(command, options, callback) {
}

// Make a shallow copy so we don't clobber the user's options object.
options = Object.assign({}, options);
Copy link
Member

Choose a reason for hiding this comment

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

i realize this isn't directly related but it would be good to avoid assigning to the argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do that pretty often everywhere throughout the code?

Copy link
Member

Choose a reason for hiding this comment

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

could be a separate pr i guess... i just figured as long as you're modifying a bunch of these you could change the argument name. non blocking.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 18, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 18, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 20, 2018
@BridgeAR
lib: switch to object spread where possible
4b7a530 
Use the object spread notation instead of using Object.assign.
It is not only easier to read it is also faster as of V8 6.8.

PR-URL: nodejs#25104
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 4b7a530 🎉

@BridgeAR BridgeAR closed this Dec 20, 2018
tshemsedinov added a commit to metarhia/impress that referenced this pull request Dec 20, 2018
@tshemsedinov
Change Object.assign to object spread
84c1fec 
Refs: nodejs/node#25104
@BridgeAR BridgeAR added the performance Issues and PRs related to the performance of Node.js. label Dec 23, 2018
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
@BridgeAR @MylesBorins
lib: switch to object spread where possible
d23d63d 
Use the object spread notation instead of using Object.assign.
It is not only easier to read it is also faster as of V8 6.8.

PR-URL: #25104
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
@BridgeAR @MylesBorins
lib: switch to object spread where possible
2fc43fb 
Use the object spread notation instead of using Object.assign.
It is not only easier to read it is also faster as of V8 6.8.

PR-URL: #25104
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
lib: switch to object spread where possible
c9781f6 
Use the object spread notation instead of using Object.assign.
It is not only easier to read it is also faster as of V8 6.8.

PR-URL: nodejs#25104
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR deleted the use-object-spread branch January 20, 2020 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@devsnek devsnek devsnek approved these changes

@JungMinu JungMinu JungMinu approved these changes

@hiroppy hiroppy hiroppy approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@BridgeAR @nodejs-github-bot @jasnell @addaleax @lpinca @targos @hiroppy @JungMinu @devsnek @lundibundi