test,benchmark: have benchmark tests confirm that only one line of output is generated per benchmark file #21046

Trott · 2018-05-30T20:46:35Z

This required:

changing benchmark tests that produced more than one line of output per benchmark file to only produce one (which is what we've always wanted anyway)
~~add a build step to MakeFile to make sure that~~ delete the misc/function_calls benchmark ~~will work; /cc @nodejs/build~~
make it so the benchmark test module checks the output to make sure each file only produces one line of results

Whoops, just realized I forgot to add the build step to vcbuild.bat. Marking as in progress for the moment until I fix that. (If someone wants to add a commit to the branch to do that, feel free to beat me to it. That would be awesome.)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

apapirovski

Amazing, thank you for working on this!!

apapirovski · 2018-05-31T06:18:36Z

test/common/benchmark.js

  child.on('exit', (code, signal) => {
    assert.strictEqual(code, 0);
    assert.strictEqual(signal, null);
+    // This bit makes sure that each benchmark file is being sent settings such
+    // that the benchmark file runs just one set of options. This helps keep the
+    // benchmark tests from taking a long time to run. THerefore, each benchmark


Capitalization issue in "Therefore"

Trott · 2018-05-31T07:23:51Z

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

Trott · 2018-05-31T07:43:02Z

Hmmm...if I'm not mistaken, there is no build step for the C++ benchmark code in vcbuild.bat.

I'll probably have to create it because I don't think this will pass on Windows without it.

Maybe someone with a Windows laptop at the Collaborator Summit would like to pair on this sometime today (or just do it without me and add your commit to this branch)? It will no doubt go a lot faster if we can build locally rather than having me guess at the write vcbuild.bat incantation, testing on CI, having it not work, rinse and repeat...

Trott · 2018-05-31T07:53:59Z

@apapirovski and anyone else: The first two commits here are the PR at #21032. It might simplify things a bit if those land first (and they should land anyway, independent of this), so feel free to review that PR and maybe give an approval for fast-tracking.

Or not, and it will all land eventually.

Trott · 2018-05-31T08:03:48Z

Argh, yeah, confirmed, fails on Windows because there's no build step for the C++ benchmark code.

joyeecheung · 2018-05-31T09:40:11Z

Instead of building misc/function_calls in the tests, I think we might as well skip it in the tests for the moment - and other addon tests to come until we work out how they should be built properly.

Trott · 2018-05-31T21:03:25Z

This is now ready to go. Discussion with other Collaborators led to the conclusion that the benchmark C++ benchmark is of little or no value and should be deleted.

@apapirovski @addaleax @joyeecheung @BridgeAR

Trott · 2018-05-31T21:04:02Z

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

addaleax · 2018-05-31T21:42:29Z

test/common/benchmark.js

+
+  let stdout = '';
+  child.stdout.on('data', (line) => {
+    const thisLine = line.toString();


Can we rather use child.stdout.setEncoding('utf8')? That way we don’t have to explicitly convert + we handle multi-byte characters properly, if that should ever be an issue

Trott · 2018-06-01T07:48:34Z

Updated with nit from @addaleax.

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

richardlau · 2018-06-01T11:41:44Z

Hmmm...if I'm not mistaken, there is no build step for the C++ benchmark code in vcbuild.bat.

... Discussion with other Collaborators led to the conclusion that the benchmark C++ benchmark is of little or no value and should be deleted.

FYI part of #21072 proposes adding a new C++ benchmark for n-api function calls.

gabrielschulhof · 2018-06-01T14:16:36Z

Measuring call-into-native is relevant for N-API, because we have to measure up to plain V8 addons. For example, #21072 has revealed a potential near-halving (49%) of the overhead of calling into a N-API addon.

Trott · 2018-06-01T14:43:30Z

Marking as blocked on the N-API comments from @richardlau and @gabrielschulhof. Thanks for the information. This will probably have to find another way to deal with these things. If either of you want to add something to Makefile and vcbuild.bat such that it builds the C++ stuff as part of make test and friends, that would be awesome. Just saying. :-D

joyeecheung · 2018-06-02T01:48:41Z

@Trott I still think it's OK to skip the addon tests for now and leave a TODO there about making the build steps work.

Prevent misc benchmark files from running more than one benchmark during tests.

Move C++ benchmark useful for NAPI to its own directory. This will isolate the benchmark so it can be excluded from testing that applies to all other benchmarks but not this one.

Check that benchmark tests are not running longer than necessary by confirming that they only produce one set of configs to report on per benchmark file.

Trott · 2018-06-15T20:17:48Z

I moved the function_call benchmark to its own directory (napi instead of misc). How's that work for people?

Trott · 2018-06-15T20:18:30Z

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

Trott · 2018-06-16T14:58:44Z

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

Trott · 2018-06-16T17:16:58Z

CI is green, but I did move that one benchmark to its own directory since the last reviews, so it would be good if people who already approved this could indicate if this is still good by them... @apapirovski @jasnell @addaleax

addaleax

Still LGTM :)

Prevent misc benchmark files from running more than one benchmark during tests. PR-URL: nodejs#21046 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Move C++ benchmark useful for NAPI to its own directory. This will isolate the benchmark so it can be excluded from testing that applies to all other benchmarks but not this one. PR-URL: nodejs#21046 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Check that benchmark tests are not running longer than necessary by confirming that they only produce one set of configs to report on per benchmark file. PR-URL: nodejs#21046 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Trott · 2018-06-19T17:38:43Z

Landed in 8944a4f...b72de3d

Prevent misc benchmark files from running more than one benchmark during tests. PR-URL: #21046 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Move C++ benchmark useful for NAPI to its own directory. This will isolate the benchmark so it can be excluded from testing that applies to all other benchmarks but not this one. PR-URL: #21046 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Check that benchmark tests are not running longer than necessary by confirming that they only produce one set of configs to report on per benchmark file. PR-URL: #21046 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label May 30, 2018

Trott added wip Issues and PRs that are still a work in progress. test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. labels May 30, 2018

apapirovski approved these changes May 31, 2018

View reviewed changes

Trott force-pushed the berlin-7 branch from 66ab91e to eddd823 Compare May 31, 2018 20:55

jasnell approved these changes May 31, 2018

View reviewed changes

addaleax approved these changes May 31, 2018

View reviewed changes

Trott force-pushed the berlin-7 branch from eddd823 to 5167196 Compare June 1, 2018 07:48

Trott added blocked PRs that are blocked by other issues or PRs. and removed wip Issues and PRs that are still a work in progress. labels Jun 1, 2018

nodejs deleted a comment from refack Jun 1, 2018

Trott force-pushed the berlin-7 branch from 5167196 to 8e50745 Compare June 2, 2018 02:44

Trott force-pushed the berlin-7 branch from 8e50745 to d8e0ee7 Compare June 10, 2018 03:38

Trott added 3 commits June 15, 2018 13:01

test: run misc benchmark only once in tests

7cb42ac

Prevent misc benchmark files from running more than one benchmark during tests.

benchmark: create napi benchmark directory

dfcc424

Move C++ benchmark useful for NAPI to its own directory. This will isolate the benchmark so it can be excluded from testing that applies to all other benchmarks but not this one.

test: check that benchmark tests are minimal

067757e

Check that benchmark tests are not running longer than necessary by confirming that they only produce one set of configs to report on per benchmark file.

Trott force-pushed the berlin-7 branch from d8e0ee7 to 067757e Compare June 15, 2018 20:17

Trott removed the blocked PRs that are blocked by other issues or PRs. label Jun 15, 2018

addaleax approved these changes Jun 16, 2018

View reviewed changes

Trott closed this Jun 19, 2018

kenny-y mentioned this pull request Jun 28, 2018

benchmark: add n-api function arguments benchmark suite #21555

Closed

3 tasks

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

Trott deleted the berlin-7 branch January 13, 2022 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test,benchmark: have benchmark tests confirm that only one line of output is generated per benchmark file #21046

test,benchmark: have benchmark tests confirm that only one line of output is generated per benchmark file #21046

Trott commented May 30, 2018 •

edited

Loading

apapirovski left a comment

apapirovski May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

joyeecheung commented May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

addaleax May 31, 2018

Trott commented Jun 1, 2018

richardlau commented Jun 1, 2018

gabrielschulhof commented Jun 1, 2018

Trott commented Jun 1, 2018

joyeecheung commented Jun 2, 2018

Trott commented Jun 15, 2018

Trott commented Jun 15, 2018

Trott commented Jun 16, 2018

Trott commented Jun 16, 2018

addaleax left a comment

Trott commented Jun 19, 2018

test,benchmark: have benchmark tests confirm that only one line of output is generated per benchmark file #21046

test,benchmark: have benchmark tests confirm that only one line of output is generated per benchmark file #21046

Conversation

Trott commented May 30, 2018 • edited Loading

Checklist

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski May 31, 2018

Choose a reason for hiding this comment

Trott commented May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

joyeecheung commented May 31, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

addaleax May 31, 2018

Choose a reason for hiding this comment

Trott commented Jun 1, 2018

richardlau commented Jun 1, 2018

gabrielschulhof commented Jun 1, 2018

Trott commented Jun 1, 2018

joyeecheung commented Jun 2, 2018

Trott commented Jun 15, 2018

Trott commented Jun 15, 2018

Trott commented Jun 16, 2018

Trott commented Jun 16, 2018

addaleax left a comment

Choose a reason for hiding this comment

Trott commented Jun 19, 2018

Trott commented May 30, 2018 •

edited

Loading