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

benchmarks: add spread operator benchmark #11227

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 7, 2017

Useful for comparing spread operator performance over time.

es/spread-bench.js millions=5 count=5 method="apply": 2.2125247919432067
es/spread-bench.js millions=5 count=10 method="apply": 1.5106532676362554
es/spread-bench.js millions=5 count=20 method="apply": 1.1835349454143682
es/spread-bench.js millions=5 count=5 method="spread": 1.7321167728499514
es/spread-bench.js millions=5 count=10 method="spread": 1.1439980896751134
es/spread-bench.js millions=5 count=20 method="spread": 0.7124136178024333
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmarks

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 7, 2017

const bench = common.createBenchmark(main, {
method: ['apply', 'spread'],
count: [5, 10, 20],
Copy link
Member

@joyeecheung joyeecheung Feb 7, 2017

Choose a reason for hiding this comment

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

We can add a config for context: ['context', 'null'] so we can benchmark against things like fn.apply(context, args), fn.call(context, ..args) and fn2 = fn.bind(context); [...] fn2.call(..args) [...] too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@jasnell jasnell force-pushed the spread-benchmark branch 3 times, most recently from 0b4fec0 to d6473e3 Compare February 7, 2017 21:12
});

function makeTest(count) {
return function test(...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to use spread here as well? It might be interesting to test with/without it in case V8 should make optimizations depending on use of spread in either/both places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's intentional but good suggestion on separating those out.

@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2017

@nodejs/benchmarking ... PTAL

@joyeecheung
Copy link
Member

I think I saw somewhere mentioned the benchmark team is more about https://benchmarking.nodejs.org/ than changes to our benchmark directory...?

@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2017

Yes, but it's always good to get benchmark focused folks looking at things like this :-)

const n = +conf.millions * 1e6;
const ctx = conf.context === 'context' ? {} : null;
var fn = makeTest(conf.count, conf.rest);
const args = Array(conf.count);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think new should be added here, or just use [].

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@jasnell
Copy link
Member Author

jasnell commented Feb 10, 2017

@mscdex PTAL!

@mscdex
Copy link
Contributor

mscdex commented Feb 10, 2017

function main(conf) {
const n = +conf.millions * 1e6;
const ctx = conf.context === 'context' ? {} : null;
var fn = makeTest(conf.count, conf.rest);
Copy link
Contributor

@mscdex mscdex Feb 10, 2017

Choose a reason for hiding this comment

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

I think the linter may be complaining about this, that it should be const. Might want to run it locally just to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

O.o .. nope, just a missing ; ... fixed!

Useful for comparing spread operator performance over time.
@mscdex
Copy link
Contributor

mscdex commented Feb 10, 2017

@mscdex
Copy link
Contributor

mscdex commented Feb 10, 2017

Linter is green. LGTM.

jasnell added a commit that referenced this pull request Feb 10, 2017
Useful for comparing spread operator performance over time.

PR-URL: #11227
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member Author

jasnell commented Feb 10, 2017

Landed in 595df9f

@jasnell jasnell closed this Feb 10, 2017
italoacasas pushed a commit that referenced this pull request Feb 13, 2017
Useful for comparing spread operator performance over time.

PR-URL: #11227
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
Useful for comparing spread operator performance over time.

PR-URL: nodejs#11227
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
Useful for comparing spread operator performance over time.

PR-URL: nodejs#11227
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants