test,benchmark: have benchmark tests confirm that only one line of output is generated per benchmark file#21046
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the build 
Trott added wip 
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
Amazing, thank you for working on this!!
test/common/benchmark.js Outdated
There was a problem hiding this comment.
Capitalization issue in "Therefore"
Trott commented May 31, 2018
Trott commented May 31, 2018
Hmmm...if I'm not mistaken, there is no build step for the C++ benchmark code in 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 |
Trott commented May 31, 2018
@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 commented May 31, 2018
Argh, yeah, confirmed, fails on Windows because there's no build step for the C++ benchmark code. |
joyeecheung commented May 31, 2018
Instead of building |
Trott commented May 31, 2018
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. |
Trott commented May 31, 2018
test/common/benchmark.js Outdated
There was a problem hiding this comment.
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 commented Jun 1, 2018
Updated with nit from @addaleax. |
richardlau commented Jun 1, 2018
FYI part of #21072 proposes adding a new C++ benchmark for n-api function calls. |
gabrielschulhof commented Jun 1, 2018
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 added blocked Trott commented Jun 1, 2018
Marking as |
joyeecheung commented Jun 2, 2018
@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 commented Jun 15, 2018
I moved the function_call benchmark to its own directory ( |
Trott commented Jun 15, 2018
Trott commented Jun 16, 2018
Trott commented Jun 16, 2018
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 |
Prevent misc benchmark files from running more than one benchmark during tests. PR-URL: nodejs#21046 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Trott commented Jun 19, 2018
Landed in 8944a4f...b72de3d |
Prevent misc benchmark files from running more than one benchmark during tests. PR-URL: #21046 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
8 participants
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 thatdelete themisc/function_callsbenchmarkwill work; /cc @nodejs/buildmake 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 asin progressfor 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), orvcbuild test(Windows) passes