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: add test for child_process benchmark #12326

Closed
wants to merge 1 commit into from
Closed

test: add test for child_process benchmark #12326

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Apr 11, 2017
@joyeecheung
Copy link
Member Author

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

@joyeecheung joyeecheung mentioned this pull request Apr 11, 2017
Closed
1 task
Trott
Trott reviewed Apr 11, 2017
View reviewed changes
benchmark/child_process/spawn-echo.js Outdated
@@ -1,12 +1,12 @@
'use strict';
var common = require('../common.js');
var bench = common.createBenchmark(main, {
thousands: [1]
len: [1000]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this change. In the other benchmarks, len is a message length in bytes I think. Here, it is a number of iterations, so len is kind of misleading because it's not a length. We tend to use n for this sort of thing instead.

I'm not opposed to this, but would be curious what others thought. @nodejs/benchmarking @mscdex

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I see that you already set n to 1 when you launch the benchmarks, so maybe this should just be n and that's that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should be more consistent and use n where possible. There are some benchmarks like this and others that use thousands or millions though. I prefer to just use n, even if the values are large, for consistency.

Copy link
Member Author

@joyeecheung joyeecheung Apr 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I think I renamed this to len because I mistook this one for another benchmark when editing :S..this should've been n, good catch!

@joyeecheung
Copy link
Member Author

Updated to use n in spawn-echo.js.

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

@joyeecheung
Copy link
Member Author

I am going to land this in 24 hours if no one objects to the update :)

@joyeecheung
Copy link
Member Author

Landed in 3d7c82b, thanks!

joyeecheung added a commit that referenced this pull request Apr 19, 2017
@joyeecheung
test: add test for child_process benchmark
3d7c82b 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@vsemozhetbyt
Copy link
Contributor

FWIW, sequential/test-benchmark-child-process hangs on some Windows CI.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2017

Oh good, it's not just me. test-benchmark-child-process definitely appears to be somewhat flaky on Windows CI.

@Trott
Copy link
Member

Trott commented Apr 19, 2017

This landed despite the added test failing on the Windows CI run for this PR. :-(

Seems to run OK on all the CI Windows types except Windows 2016. Maybe yes.exe isn't installed there? @nodejs/build

@Trott Trott mentioned this pull request Apr 19, 2017
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Apr 19, 2017

Although I don't think it would hang if that's the case...it would exit with ENOENT, I think.

@joyeecheung
Copy link
Member Author

Oh no, sorry for the trouble, should've checked the new CI :(

evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@joyeecheung @evanlucas
test: add test for child_process benchmark
80e8451 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@joyeecheung @evanlucas
test: add test for child_process benchmark
05e1404 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@joyeecheung @evanlucas
test: add test for child_process benchmark
e919941 
PR-URL: #12326
Ref: #12068
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@refack refack mentioned this pull request May 4, 2017
Closed
@gibfahn
Copy link
Member

gibfahn commented May 16, 2017 •
edited
Loading

Opting to let this bake a while longer. This should land with #12518 (and possibly #12561)

@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 16, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@mscdex mscdex mscdex left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@aqrln aqrln aqrln approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@joyeecheung @vsemozhetbyt @jasnell @Trott @gibfahn @mscdex @cjihrig @aqrln @MylesBorins @nodejs-github-bot