benchmarks: add spread operator benchmark #11227

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@jasnell
Member

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

benchmark/es/spread-bench.js
+
+const bench = common.createBenchmark(main, {
+ method: ['apply', 'spread'],
+ count: [5, 10, 20],

This comment has been minimized.

@joyeecheung

joyeecheung Feb 7, 2017

Member

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.

@joyeecheung

joyeecheung Feb 7, 2017

Member

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.

This comment has been minimized.

@jasnell

jasnell Feb 7, 2017

Member

Done!

@jasnell

jasnell Feb 7, 2017

Member

Done!

benchmark/es/spread-bench.js
+});
+
+function makeTest(count) {
+ return function test(...args) {

This comment has been minimized.

@mscdex

mscdex Feb 7, 2017

Contributor

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?

@mscdex

mscdex Feb 7, 2017

Contributor

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?

This comment has been minimized.

@jasnell

jasnell Feb 7, 2017

Member

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

@jasnell

jasnell Feb 7, 2017

Member

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Feb 9, 2017

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 9, 2017

Member

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

Member

joyeecheung commented Feb 9, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 9, 2017

Member

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

Member

jasnell commented Feb 9, 2017

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

benchmark/es/spread-bench.js
+ const n = +conf.millions * 1e6;
+ const ctx = conf.context === 'context' ? {} : null;
+ var fn = makeTest(conf.count, conf.rest);
+ const args = Array(conf.count);

This comment has been minimized.

@mscdex

mscdex Feb 9, 2017

Contributor

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

@mscdex

mscdex Feb 9, 2017

Contributor

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

This comment has been minimized.

@jasnell

jasnell Feb 10, 2017

Member

Updated!

@jasnell

jasnell Feb 10, 2017

Member

Updated!

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Feb 10, 2017

@mscdex PTAL!

@mscdex

This comment has been minimized.

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

This comment has been minimized.

@mscdex

mscdex Feb 10, 2017

Contributor

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

@mscdex

mscdex Feb 10, 2017

Contributor

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

This comment has been minimized.

@jasnell

jasnell Feb 10, 2017

Member

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

@jasnell

jasnell Feb 10, 2017

Member

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

benchmarks: add spread operator benchmark
Useful for comparing spread operator performance over time.
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 10, 2017

Contributor

Linter is green. LGTM.

Contributor

mscdex commented Feb 10, 2017

Linter is green. LGTM.

jasnell added a commit that referenced this pull request Feb 10, 2017

benchmarks: add spread operator benchmark
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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 10, 2017

Member

Landed in 595df9f

Member

jasnell commented Feb 10, 2017

Landed in 595df9f

@jasnell jasnell closed this Feb 10, 2017

italoacasas added a commit that referenced this pull request Feb 13, 2017

benchmarks: add spread operator benchmark
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 added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

benchmarks: add spread operator benchmark
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 added a commit to KryDos/node that referenced this pull request Feb 25, 2017

benchmarks: add spread operator benchmark
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