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

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

Closed
wants to merge 1 commit into from

Conversation

suryagh
Copy link
Contributor

@suryagh suryagh commented Jun 8, 2016

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 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
Copy link
Contributor

cjihrig commented Jun 8, 2016

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
Copy link
Contributor

mscdex commented Jun 8, 2016

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

@suryagh
Copy link
Contributor Author

suryagh commented Jun 8, 2016

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

Here is the test script and the result.

@suryagh
Copy link
Contributor Author

suryagh commented Jun 8, 2016

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

@suryagh
Copy link
Contributor Author

suryagh commented Jun 8, 2016

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
Copy link
Contributor

cjihrig commented Jun 8, 2016

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
Copy link
Contributor Author

suryagh commented Jun 8, 2016

@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
Copy link
Contributor

cjihrig commented Jun 9, 2016

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

cc: @nodejs/benchmarking

@suryagh
Copy link
Contributor Author

suryagh commented Jun 9, 2016

@cjihrig will do.

@suryagh suryagh closed this Jun 9, 2016
@suryagh
Copy link
Contributor Author

suryagh commented Jun 10, 2016

#7255

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