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

benchmark: util._extend vs object.assign #7255

Closed
wants to merge 4 commits into from
Closed

Conversation

suryagh
Copy link
Contributor

@suryagh suryagh commented Jun 10, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

benchmark

Description of change

To copy the values of all enumerable properties from-
a source object to a target object, node still use-
util._extend, though newer standard Object.assign
is available. This is because util._extend is found to
be faster than Object.assign. This benchmark test is
to keep track of how performance compare.

To copy the values of all enumerable own properties from-
a source object to a target object, node still use-
`util._extend`, though newer standard `Object.assign`
is available. This is because `util._extend` is found to
be faster than `Object.assign`. This benchmark test is
to keep track of how performance compare.
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jun 10, 2016
@mscdex mscdex added util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency. labels Jun 10, 2016

const bench = common.createBenchmark(main, {
type: ['util._extend', 'Object.assign',
'util._extend', 'Object.assign'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there duplicates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

if (conf.type === 'extend') {
fn = util._extend;
v8command = '%OptimizeFunctionOnNextCall(util._extend)';
} else if (conf.type === 'assign') {
Copy link
Member

Choose a reason for hiding this comment

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

FYI: https://github.com/nodejs/node/blob/master/benchmark/common.js#L255

the benchmark/common.js now has a method for handling the details of v8 optimization for you. e.g.

function myMethod(a,b) {
  /** ... **/
}
common.v8ForceOptimization(myMethod, 'a', 'b');
myMethod('a', 'b');

@jasnell
Copy link
Member

jasnell commented Jun 20, 2016

Couple of minor nits but LGTM if @mscdex is happy with it.

v8command = '%OptimizeFunctionOnNextCall(util._extend)';
} else if (conf.type === 'assign') {
fn = Object.assign;
//Object.assign is built-in, cannot be optimized
Copy link
Contributor

Choose a reason for hiding this comment

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

Space needed after //.

@mscdex
Copy link
Contributor

mscdex commented Jun 20, 2016

One minor style nit, but otherwise LGTM

@suryagh
Copy link
Contributor Author

suryagh commented Jun 20, 2016

Fixed the minor style.

jasnell pushed a commit that referenced this pull request Jun 27, 2016
To copy the values of all enumerable own properties from-
a source object to a target object, node still use-
`util._extend`, though newer standard `Object.assign`
is available. This is because `util._extend` is found to
be faster than `Object.assign`. This benchmark test is
to keep track of how performance compare.

PR-URL: #7255
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Jun 27, 2016

Landed in 6abb06f

@jasnell jasnell closed this Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
To copy the values of all enumerable own properties from-
a source object to a target object, node still use-
`util._extend`, though newer standard `Object.assign`
is available. This is because `util._extend` is found to
be faster than `Object.assign`. This benchmark test is
to keep track of how performance compare.

PR-URL: #7255
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants