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, test: test the HTTP benchmark with a dummy benchmarker #12121

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@joyeecheung
Member

joyeecheung commented Mar 29, 2017

This is a retake of #12025, it basically just goes with the last option in #12025 (comment), makes a dummy http benchmarker to connect to the benchmarked http server once, and sets all the numeric benchmark configurations to the minimal.

  • The first commit also refactors _http-benchmarks a bit to use ES6 classes.
  • The second commit also moves http/_http_simple.js to fixtures/simple-http-server.js since it is not a benchmark itself

This adds about 10s to the test running time on my mac (i7-4770HQ CPU @ 2.20GHz) (as comparison, test-benchmark-net.js runs for about 5s)

Refs: #12068

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)

test, benchmark

@vsemozhetbyt vsemozhetbyt added the http label Mar 29, 2017

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Mar 29, 2017

Member

It's timing out on pi1-raspbian-wheezy..should we skip this test on these slower devices, or set a larger timeout for them? (not sure how to do this)

Member

joyeecheung commented Mar 29, 2017

It's timing out on pi1-raspbian-wheezy..should we skip this test on these slower devices, or set a larger timeout for them? (not sure how to do this)

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Mar 29, 2017

Member

There is also another way to fix the timeout: test the benchmarks one by one in multiple tests using --filter of benchmark/run.js instead of checking them all in one test.

Member

joyeecheung commented Mar 29, 2017

There is also another way to fix the timeout: test the benchmarks one by one in multiple tests using --filter of benchmark/run.js instead of checking them all in one test.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 29, 2017

Member

Love it! I'd be OK with "fixing" the Pi timing out problem by not running this test on Pi. Since it's just the Pi 1 that is failing you can do this:

if (!common.enoughTestMem) {
  common.skip('Insufficient memory for HTTP benchmark test');
  return;
}
Member

Trott commented Mar 29, 2017

Love it! I'd be OK with "fixing" the Pi timing out problem by not running this test on Pi. Since it's just the Pi 1 that is failing you can do this:

if (!common.enoughTestMem) {
  common.skip('Insufficient memory for HTTP benchmark test');
  return;
}
Show outdated Hide outdated benchmark/_dummy-benchmarker.js
num: [1, 4, 8, 16],
size: [1, 64, 256],
n: [1, 4, 8, 16],
len: [1, 64, 256],

This comment has been minimized.

@jasnell

jasnell Mar 29, 2017

Member

why change these?

@jasnell

jasnell Mar 29, 2017

Member

why change these?

This comment has been minimized.

@joyeecheung

joyeecheung Mar 30, 2017

Member

So test-benchmark-http.js can set the parameters for it (otherwise it will need to add --set num=1 --set size=1...also I think it's better to keep the configuration names consistent)

@joyeecheung

joyeecheung Mar 30, 2017

Member

So test-benchmark-http.js can set the parameters for it (otherwise it will need to add --set num=1 --set size=1...also I think it's better to keep the configuration names consistent)

This comment has been minimized.

@joyeecheung

joyeecheung Mar 30, 2017

Member

Also if we want to do #12068, doing this to other benchmarks too will make it eaiser..

@joyeecheung

joyeecheung Mar 30, 2017

Member

Also if we want to do #12068, doing this to other benchmarks too will make it eaiser..

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

Renamed dummy-benchmarker to test-trouble-benchmarker and skip test on Pi 1..PTAL, thanks @jasnell @Trott

New CI: https://ci.nodejs.org/job/node-test-pull-request/7150/

Member

joyeecheung commented Apr 1, 2017

Renamed dummy-benchmarker to test-trouble-benchmarker and skip test on Pi 1..PTAL, thanks @jasnell @Trott

New CI: https://ci.nodejs.org/job/node-test-pull-request/7150/

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

node-test-commit-osx took 2h30m to run..although it doesn' seem to be related to this PR since https://ci.nodejs.org/job/node-test-commit-osx/8734/ and https://ci.nodejs.org/job/node-test-commit/8819/ took more than 2 hours too?

Member

joyeecheung commented Apr 1, 2017

node-test-commit-osx took 2h30m to run..although it doesn' seem to be related to this PR since https://ci.nodejs.org/job/node-test-commit-osx/8734/ and https://ci.nodejs.org/job/node-test-commit/8819/ took more than 2 hours too?

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung
Member

joyeecheung commented Apr 1, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 1, 2017

Member

I think you meant to rename it test-double/TestDouble rather than test-trouble/testTrouble...

Member

Trott commented Apr 1, 2017

I think you meant to rename it test-double/TestDouble rather than test-trouble/testTrouble...

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

It's double now, thanks for catching that @vsemozhetbyt @Trott :D
new CI: https://ci.nodejs.org/job/node-test-pull-request/7152/

Member

joyeecheung commented Apr 1, 2017

It's double now, thanks for catching that @vsemozhetbyt @Trott :D
new CI: https://ci.nodejs.org/job/node-test-pull-request/7152/

@Trott

Trott approved these changes Apr 1, 2017

LGTM if CI is green

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 5, 2017

Member

@jasnell Is @joyeecheung's answer to your question "why change the config property names" sufficient? If so can you update your review so this can be unblocked?

@joyeecheung Can you rebase?

(I'd really like to see this land. :-D )

Member

Trott commented Apr 5, 2017

@jasnell Is @joyeecheung's answer to your question "why change the config property names" sufficient? If so can you update your review so this can be unblocked?

@joyeecheung Can you rebase?

(I'd really like to see this land. :-D )

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 6, 2017

Member

Yes. It's fine. I can't clear my review from the mobile view. This lgtm

Member

jasnell commented Apr 6, 2017

Yes. It's fine. I can't clear my review from the mobile view. This lgtm

No longer accurate

joyeecheung added some commits Mar 29, 2017

benchmark: add dummy HTTP benchmarker
Refactor benchmark/_http-benchmarkers.js and add a dummy
HTTP benchmarker for testing.
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
Member

joyeecheung commented Apr 8, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 8, 2017

Member

CI failures are unrelated. LGTM.

Member

Trott commented Apr 8, 2017

CI failures are unrelated. LGTM.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 9, 2017

Member

Landed in c095394...2d3d4cc, thanks!

Member

joyeecheung commented Apr 9, 2017

Landed in c095394...2d3d4cc, thanks!

@joyeecheung joyeecheung closed this Apr 9, 2017

joyeecheung added a commit that referenced this pull request Apr 9, 2017

benchmark: add test double HTTP benchmarker
Refactor benchmark/_http-benchmarkers.js and add a test double
HTTP benchmarker for testing.

PR-URL: #12121
Refs: #12068
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung added a commit that referenced this pull request Apr 9, 2017

benchmark: control HTTP benchmarks run time
PR-URL: #12121
Refs: #12068
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung added a commit that referenced this pull request Apr 9, 2017

test: add http benchmark test
PR-URL: #12121
Refs: #12068
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@Trott Trott referenced this pull request Apr 9, 2017

Closed

test,benchmark: enable minimal CI for http bench #12025

3 of 3 tasks complete
@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas
Member

italoacasas commented Apr 10, 2017

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 11, 2017

Member

Some of the changes depend on #10558, but should still be backportable. Doing a backport now...

Member

joyeecheung commented Apr 11, 2017

Some of the changes depend on #10558, but should still be backportable. Doing a backport now...

joyeecheung added a commit to joyeecheung/node that referenced this pull request Apr 11, 2017

benchmark: add test double HTTP benchmarker
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 pull request Apr 11, 2017

joyeecheung added a commit to joyeecheung/node that referenced this pull request Apr 11, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 8, 2017

Member

@joyeecheung do you think it makes sense to backport this to Node 6?

If it is backported it should land with #13109 and #13390

Member

gibfahn commented Jun 8, 2017

@joyeecheung do you think it makes sense to backport this to Node 6?

If it is backported it should land with #13109 and #13390

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Jun 10, 2017

Member

@gibfahn The new benchmark suite does not exist in 6.x so it can not be backported.

Member

joyeecheung commented Jun 10, 2017

@gibfahn The new benchmark suite does not exist in 6.x so it can not be backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment