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.

Sign up for GitHub

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

Jump to bottom

test the benchmarks at one iteration #12068

Closed
Fishrock123 opened this issue Mar 27, 2017 · 11 comments
Closed

test the benchmarks at one iteration #12068

Fishrock123 opened this issue Mar 27, 2017 · 11 comments
Labels
benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests.

Comments

@Fishrock123
Copy link
Contributor

Of course we don't want to "actually" run the benchmarks during a test run. However, the benchmark runner already is able to set how many iterations the benchmarks run, and we could probably extend that even more.

As such, I don't see much reason to not run most of the benchmarks during a normal test run, with e.g. only one iteration set.

Or, at the very least perhaps we could have some sort of special CI job that does that, to be run, say, weekly.

cc @nodejs/testing @nodejs/benchmarking
@Fishrock123 Fishrock123 added benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests. labels Mar 27, 2017
@vsemozhetbyt
Copy link
Contributor

Refs: #11979, #12030

@Fishrock123
Copy link
Contributor Author

Ah, only for net. Yeah, some adjustments will need to be made to the benchmarks in general.

e.g. some still run iterations even when it is set to 1: https://github.com/nodejs/node/blob/master/benchmark/timers/timers-cancel-pooled.js#L10

@vsemozhetbyt vsemozhetbyt mentioned this issue Mar 27, 2017
Closed
1 task
@Trott
Copy link
Member

Trott commented Mar 28, 2017

Refs: #12025

@joyeecheung joyeecheung mentioned this issue Mar 29, 2017
Closed
3 tasks
joyeecheung added a commit that referenced this issue Apr 9, 2017
@joyeecheung
benchmark: add test double HTTP benchmarker
a3e71a8 
Refactor benchmark/_http-benchmarkers.js and add a test double
HTTP benchmarker for testing.

PR-URL: #12121
Refs: #12068
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Apr 9, 2017
@joyeecheung
benchmark: control HTTP benchmarks run time
3e3414f 
PR-URL: #12121
Refs: #12068
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Apr 9, 2017
@joyeecheung
test: add http benchmark test
2d3d4cc 
PR-URL: #12121
Refs: #12068
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Apr 11, 2017
@joyeecheung
benchmark: add test double HTTP benchmarker
9432cae 
Refactor benchmark/_http-benchmarkers.js and add a test double
HTTP benchmarker for testing.

PR-URL: nodejs#12121
Refs: nodejs#12068
joyeecheung added a commit to joyeecheung/node that referenced this issue Apr 11, 2017
@joyeecheung
benchmark: control HTTP benchmarks run time
b59c461 
PR-URL: nodejs#12121
Refs: nodejs#12068
joyeecheung added a commit to joyeecheung/node that referenced this issue Apr 11, 2017
@joyeecheung
test: add http benchmark test
d60fa5b 
PR-URL: nodejs#12121
Refs: nodejs#12068
@joyeecheung joyeecheung mentioned this issue Apr 11, 2017
Closed
3 tasks
joyeecheung added a commit that referenced this issue Apr 19, 2017
@joyeecheung
test: add test for child_process benchmark
3d7c82b 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
@joyeecheung @evanlucas
test: add test for child_process benchmark
80e8451 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
evanlucas pushed a commit that referenced this issue May 1, 2017
@joyeecheung @evanlucas
test: add test for child_process benchmark
05e1404 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
evanlucas pushed a commit that referenced this issue May 2, 2017
@joyeecheung @evanlucas
test: add test for child_process benchmark
e919941 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@refack
Copy link
Contributor

refack commented May 4, 2017

@Fishrock123 were you interested in the results, or do you just want to make sure the benchmark suite doesn't regress? Because AFAIK #12326 & #12121 currently only give green/red...

@joyeecheung
Copy link
Member

@refack I think the OP means we need to make sure that we can run the benchmarks on the platforms we support without throwing errors? Like to make sure the suite won't be broken by a PR without a CI run.

@Fishrock123
Copy link
Contributor Author

I want to make sure the benchmarks even run.

@refack
Copy link
Contributor

refack commented May 8, 2017

Cool.
What's missing? Can I help?

@targos
Copy link
Member

targos commented Sep 3, 2017

@Trott has been working on that: #14812, #14951, #14763, #14728 and maybe others. Can we consider this resolved?

@Trott
Copy link
Member

Trott commented Sep 4, 2017 •
edited
Loading

Can we consider this resolved?

@targos I'd say not quite yet, although I'm getting there. Between what's currently in the code base and what I now have PRs open for, there are 16 tests done and 15 more to go.

Here are the 15 that remain:

  • dgram
  • es
  • fs
  • http2
  • misc
  • module
  • querystring
  • streams
  • string_decoder
  • tls
  • url
  • util
  • v8
  • vm

I'd been planning on doing all of these, but if someone wants to pick some off before I get around to it, awesome. (Maybe just drop a note here so I don't bother with whatever you're working on. Also, might be good to wait for #15004 to land. Should greatly simplify things.)

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

@nodejs/testing @nodejs/benchmarking ... ping... would be great to get this one completed.

Trott pushed a commit to lundibundi/node that referenced this issue Aug 18, 2018
@lundibundi @Trott
test: add v8 benchmark test
bfe14eb 
Refs: nodejs#12068
Trott pushed a commit to lundibundi/node that referenced this issue Aug 18, 2018
@lundibundi @Trott
test: add vm benchmark test
59fd07a 
Refs: nodejs#12068
Trott pushed a commit to lundibundi/node that referenced this issue Aug 18, 2018
@lundibundi @Trott
test: add streams benchmark test
f56e2d6 
Refs: nodejs#12068
addaleax pushed a commit that referenced this issue Aug 24, 2018
@lundibundi @addaleax
test: add v8 benchmark test
cac4e6d 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Aug 24, 2018
@lundibundi @addaleax
test: add vm benchmark test
5d703ec 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Aug 24, 2018
@lundibundi @addaleax
test: add streams benchmark test
902fd40 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Aug 27, 2018
@lundibundi @addaleax
test: add v8 benchmark test
a196107 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Aug 27, 2018
@lundibundi @addaleax
test: add vm benchmark test
8e91dbd 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Aug 27, 2018
@lundibundi @addaleax
test: add streams benchmark test
865e43e 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
@lundibundi @targos
test: add v8 benchmark test
400aac8 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
@lundibundi @targos
test: add vm benchmark test
ef60a8d 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
@lundibundi @targos
test: add streams benchmark test
1e7deb7 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
@lundibundi @targos
test: add v8 benchmark test
594b237 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
@lundibundi @targos
test: add vm benchmark test
caaac3f 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
@lundibundi @targos
test: add streams benchmark test
c91472e 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@apapirovski
Copy link
Member

This is done now. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@apapirovski @refack @jasnell @Trott @Fishrock123 @targos @joyeecheung @vsemozhetbyt