FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: replace util._extend with object.assign by suryagh · Pull Request #7208 · nodejs/node · GitHub

/ node Public

lib: replace util._extend with object.assign - #7208

Closed
suryagh wants to merge 1 commit into
nodejs:masterfrom
suryagh:objectassign
Closed

lib: replace util._extend with object.assign#7208
suryagh wants to merge 1 commit into
nodejs:masterfrom
suryagh:objectassign

Conversation

suryagh commented Jun 8, 2016
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

http, tls, child-process, cluster, domain, fs, https, tty

Description of change

Object.assign is built-in and has some performance advantage-
over the polyfill.

One thing to keep an eye is that _extend used to fail silently-
while assign throws an error. Let me know if there are any concerns-
or if the pr need to be split based on the components.

`Object.assign` is built-in and has slight performance advantage-
 over the polyfill `_extend`.

One thing to keep an eye is that `._extend` used  to silently fails-
while `assign` throws an error.
nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 8, 2016

cjihrig commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

Object.assign is built-in and has some performance advantage-
over the polyfill.

Do you have some benchmarks to back that up? That wasn't the case, as of #4593.

mscdex commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

Also see this similar PR that was submitted just the other week.

suryagh commented Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig with large sample I can consistently see over 10% performance gain using Object.assign.

Here is the test script and the result.

suryagh commented Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig I just made a couple of minor changes to the script, however the results remain around the same.

suryagh commented Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

I will close this pr in favour of any one of the similar earlier prs referenced here.

However, would like to hear if anyone has any thoughts on the benchmark findings using the attached script.

cjihrig commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

However, would like to hear if anyone has any thoughts on the benchmark findings using the attached script.

I think it was the nature of the data you were operating on. I switched to util._extend(extend, process.env); and Object.assign(extend, process.env); and saw a much bigger difference, in favor of util._extend().

$ ./node cji.js 
_extend took:: 186755.394ms
oassign took:: 319979.233ms
_extend took:: 200674.158ms
oassign took:: 346792.866ms
^C

It might be good to have a benchmark in the repo for this, since it keeps coming up.

suryagh commented Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig this is interesting. With your process.env approach, except the first iteration of the while loop, the source and destination objects are always similar structured objects. And in this case _extend() looks to be much faster. Seems optimizer (v8?) is able to detect this.
However, when the source and destination objects are disjoint structured, i.e, every property of the src needs to be copied to the dest, then Object.assign() looks to be slightly faster.

cjihrig commented Jun 9, 2016

Copy link
Copy Markdown
Contributor

@suryagh can I suggest opening a PR to create a benchmark for this?

cc: @nodejs/benchmarking

suryagh commented Jun 9, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig will do.

suryagh closed this Jun 9, 2016

suryagh commented Jun 10, 2016

Copy link
Copy Markdown
Contributor Author

#7255

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL