test, benchmark: create shared runBenchmark func #15004

maclover7 · 2017-08-24T07:31:02Z

Created from discussion starting at #14951 (comment). This is a proof of concept PR, and the exact implementation details can be tweaked, but I think this is generally what the API should look like. A few open points are below.

Needed for merge: all resolved

ESLint is raising with Mandatory module "common" must be loaded required-modules right now -- should files matching test-benchmark-*.jsneed to require common anymore? resolved

Can probably be handled after merge:

Does this new helper method need to be documented in test/common/README.md?
To a novice, it seems like n=1 is being everywhere except a few places -- should this become an included-by-default argv option in the new run method?
exchange args array option for an object instead

Checklist

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

Affected core subsystem(s)

test, benchmark

benjamingr · 2017-08-24T07:32:31Z

Can we see a run of the results before and after?

maclover7 · 2017-08-24T07:39:39Z

Can we see a run of the results before and after?

Not sure what you mean -- this PR doesn't affect the output of the benchmarks themselves, it just refactors the way they are run 😬

benjamingr · 2017-08-24T07:42:05Z

Not sure what you mean -- this PR doesn't affect the output of the benchmarks themselves, it just refactors the way they are run 😬

Precisely, which is why I'd expect a benchmark before and after to return the same results and it would be a good sanity check that we did not change anything in the measuring code (in how the JIT looks at the code for example).

jasnell · 2017-08-24T15:27:37Z

test/common/benchmarks.js

I believe this will need a /* eslint-disable required-modules */ at the top to pass linting.
Please run make lint to make sure :-)

jasnell

Minor nit but I like it! Thank you for this.

joyeecheung · 2017-08-24T16:45:04Z

@maclover7 I think you can verify this PR does not change how the benchmarks are tested by running something like:

for test in test/**/test-benchmark-*.js; do ./node $test; done

joyeecheung · 2017-08-24T16:51:04Z

Also:

ESLint is raising with Mandatory module "common" must be loaded required-modules right now -- should files matching test-benchmark-*.jsneed to require common anymore?

Yes, we need to require('../common') to make sure no global variables are leaked during the test, .etc

Does this new helper method need to be documented in test/common/README.md?

That would be great but I would not think this is a blocker for this PR.

maclover7 · 2017-08-24T19:22:21Z

Here are two diffs of benchmark runs (thank you for the helpful bash script @joyeecheung!!) 1, 2. It does seem like the numbers vary a little bit even between runs on the same git branches, but I'm not sure if that's due to the changes here.

Trott · 2017-08-24T23:01:00Z

Totally minor nit that can be ignored if you want: I'd prefer benchmark as the name for the module rather than benchmarks.

The reason is that if everything is a singular noun, you never have to think about "Hmmm, is the name of the module benchmark or benchmarks? Let me go look." It's always going to be benchmark with no thinking necessary.

Of course, that ship has sailed already because I neglected to say something about the fixtures module. (A PR that changes it to fixture would be fine with me, but others might reject it as pointless churn.)

joyeecheung · 2017-08-25T03:17:41Z

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

maclover7 · 2017-08-25T06:07:52Z

Totally minor nit that can be ignored if you want: I'd prefer benchmark as the name for the module rather than benchmarks.

@Trott while reading your great comment I actually came up with a different reason for renaming to benchmark -- the function exported is called runBenchmark, not runBenchmarks 🙃

BridgeAR

I like this approach a lot. LGTM

BridgeAR · 2017-08-25T06:16:48Z

Just one suggestion: I think it makes sense to move the tests into one file as it is only very little code now.

Trott · 2017-08-25T13:47:46Z

Just one suggestion: I think it makes sense to move the tests into one file as it is only very little code now.

@BridgeAR Logically, that makes sense, but practically, that will likely result in a test that times out a lot on CI. These tests individually can take several seconds to run each. I would keep them separate.

joyeecheung · 2017-08-29T11:16:32Z

Previous CI is incomplete. New CI: https://ci.nodejs.org/job/node-test-pull-request/9866/

jasnell · 2017-08-29T17:03:11Z

I'd like to get signoff from @Trott before this lands.

Trott · 2017-09-01T15:10:25Z

I'd like to get signoff from @Trott before this lands.

LGTM

Trott · 2017-09-01T15:11:03Z

Oh, wait, I take it back! It needs a change.

Trott

Code LGTM. The new benchmark module needs documentation. Can you at least minimally stub something out in test/common/README.md? There are already examples there that you can use as a model.

maclover7 · 2017-09-01T16:21:27Z

@Trott updated

refack · 2017-09-01T23:56:48Z

Sorry to come in late, so @maclover7 take this as a non blocking request/question.
Why not have this as a method on common so you could do

const { runBenchmark } = require('./common');

Also I think a more appropriate name would be testBenchmark since the main motivation is to sanity test the benchmarks code, and we ignore the actual results.

refack · 2017-09-02T00:02:56Z

Also folding the code into index.js will allow the use of a future patched fork as discussed in #14845

maclover7 · 2017-09-02T00:10:04Z

@refack no problem :)

Why not have this as a method on common

I think the original idea was that common was only for helper functions that could be used throughout the test suite, and since this function is benchmark specific, it seemed okay to create a separate module for it. (see also discussion beginning at #14951 (comment))

Also folding the code into index.js will allow the use of a future patched fork as discussed in

If I'm reading that PR right, I don't think the two should conflict? Whatever is wanted for process.env is passed as an object, so theoretically that could be anything. Either way, the specifics of the API can always be changed in a later PR, this one is just meant to be a first step :)

refack · 2017-09-02T00:24:12Z

Also folding the code into index.js will allow the use of a future patched fork as discussed in

If I'm reading that PR right, I don't think the two should conflict? Whatever is wanted for process.env is passed as an object, so theoretically that could be anything. Either way, the specifics of the API can always be changed in a later PR, this one is just meant to be a first step :)

Originally #14845 introduced a common.envPlus() helper, but we didn't reach consensus on that. But I'm still considering a patched common.fork.

I'd be happy to hear some of the other people's feedback on merging the modules, even irrespective of reusable code.
It just seems that each of the files currently has this patterns:

require('../common');

const runBenchmark = require('../common/benchmark');

and folding the module in would allow us to do:

const { runBenchmark } = require('./common');

instead.

ian-perkins · 2017-09-02T11:37:52Z

I added test/parallel/test-benchmark-dns.js as part of #14936

Will you also update it in this PR?

TimothyGu

LGTM. A few nits though.

TimothyGu · 2017-09-02T14:37:37Z

test/common/benchmark.js

Nit: a for-of loop could be used here, and the argv.push lines can be coalesced.

TimothyGu · 2017-09-02T14:40:11Z

test/common/README.md

Hmm, I think a better name for the parameter may be "settings"? Also the environment variable seems to be a misnomer, as does the "key/value pair" verbiage which sounds like an array (though the example definitely helps for that case). The "Array" type annotation could be changed to string[] to better represent the member types.

Hmm, I was thinking "args" because this just does --set for whatever is passed in (args as in command line args). This argument is semi confusing, but I am planning on submitting a PR after this is merged to convert this argument to be an Object so that way it's easier to understand -- using the array for right now is just to avoid a giant diff :)

FWIW it's called configs in createBenchmark, though in a context like this it should be config.

configs {Object} The benchmark parameters. createBenchmark will run all
possible combinations of these parameters, unless specified otherwise.
Each configuration is a property with an array of possible values.
Note that the configuration values can only be strings or numbers.

Trott · 2017-09-02T14:52:44Z

I'm definitely -1 on importing common via the smaller module and relaxing the lint rule. Importing common should be explicit in the test. @refack Can you clarify what problem your trying to solve with the suggestion?

refack · 2017-09-02T14:56:47Z

[again comments are non-blocking]
This seems fragile (lint-able, but surprising), and requires understanding of the ../common mechanism.

require('../common');

const runBenchmark = require('../common/benchmark');

and folding the module in would allow us to do:

const { runBenchmark } = require('./common');

instead.

But explicitivity is a strong argument, and the lint rule will protect from forgetting to require('../common');

Trott · 2017-09-02T15:16:39Z

Ah, I see! Yeah, maybe the thing to do eventually is create a node_modules directory and put common inside it along side the other modules rather than having it look like a parent module. We'd want to enable linting for that node_modules directory unlike all the others. And having it there without a package.json might be confusing.
¯\(ツ)/¯ Maybe publish the modules to npm eventually. :-D

refack · 2017-09-02T18:17:29Z

CI: https://ci.nodejs.org/job/node-test-commit/12149/

Trott · 2017-09-04T05:57:29Z

This should be land-able but CI has been Not Fun the last few days. Hopefully that can get straightened out Tuesday if not sooner, but in the meantime, @maclover7, would you mind rebasing to get rid of the conflict?

Mostly shared/duplicated logic between all benchmark test files, so creating a new common module to store it.

maclover7 · 2017-09-08T21:22:51Z

@Trott sorry for delay, rebased. read to land?

BridgeAR · 2017-09-09T19:43:19Z

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

refack · 2017-09-11T15:29:50Z

Landed in 640b206
D.R.Y. FTW!

Mostly shared/duplicated logic between all benchmark test files, so creating a new common module to store it. PR-URL: #15004 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Mostly shared/duplicated logic between all benchmark test files, so creating a new common module to store it. PR-URL: nodejs#15004 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Mostly shared/duplicated logic between all benchmark test files, so creating a new common module to store it. PR-URL: #15004 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-10-17T04:58:38Z

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

Trott · 2017-10-18T02:54:26Z

I think it's important that this one be backported. I'm going to change the label to backport-requested. Hopefully @maclover7 can do it, but if not, I or someone else can step in to do it.

Trott · 2017-10-18T02:55:22Z

Oh, wait, it landed on 8.x already. The backport would be for 6.x. I'd still prefer to see it than not, but I don't feel so strongly that I'm going to change the label. :-D

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 24, 2017

mscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Aug 24, 2017

jasnell reviewed Aug 24, 2017

View reviewed changes

jasnell approved these changes Aug 24, 2017

View reviewed changes

BridgeAR approved these changes Aug 25, 2017

View reviewed changes

joyeecheung approved these changes Aug 29, 2017

View reviewed changes

Trott requested changes Sep 1, 2017

View reviewed changes

Trott approved these changes Sep 1, 2017

View reviewed changes

refack approved these changes Sep 1, 2017

View reviewed changes

TimothyGu reviewed Sep 2, 2017

View reviewed changes

refack self-assigned this Sep 2, 2017

Trott mentioned this pull request Sep 4, 2017

test the benchmarks at one iteration #12068

Closed

maclover7 added 6 commits September 8, 2017 17:22

test, benchmark: create shared runBenchmark func

7ab1619

Mostly shared/duplicated logic between all benchmark test files, so creating a new common module to store it.

[squash] require common + convert forgotten arrays benchmarks

a6395a9

[squash] fix typo

ae90f1f

[squash] rename benchmarks to benchmark

4388c22

[squash] add initial documentation

3e5e2e2

[squash] convert test-benchmark-dns

b3f4c41

BridgeAR approved these changes Sep 9, 2017

View reviewed changes

refack closed this Sep 11, 2017

maclover7 deleted the jm-common-benchmark branch September 11, 2017 21:37

maclover7 mentioned this pull request Sep 11, 2017

test: convert buffer benchmark to runBenchmark #15349

Closed

3 tasks

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

refack removed their assignment Oct 12, 2018

Uh oh!

test, benchmark: create shared runBenchmark func #15004

test, benchmark: create shared runBenchmark func #15004

Uh oh!

Conversation

maclover7 commented Aug 24, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

benjamingr commented Aug 24, 2017

Uh oh!

maclover7 commented Aug 24, 2017

Uh oh!

benjamingr commented Aug 24, 2017

Uh oh!

jasnell Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Aug 24, 2017

Uh oh!

joyeecheung commented Aug 24, 2017

Uh oh!

maclover7 commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

joyeecheung commented Aug 25, 2017

Uh oh!

maclover7 commented Aug 25, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 25, 2017

Uh oh!

Trott commented Aug 25, 2017

Uh oh!

joyeecheung commented Aug 29, 2017

Uh oh!

jasnell commented Aug 29, 2017

Uh oh!

Trott commented Sep 1, 2017

Uh oh!

Trott commented Sep 1, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Sep 1, 2017

Uh oh!

refack commented Sep 1, 2017

Uh oh!

refack commented Sep 2, 2017

Uh oh!

maclover7 commented Sep 2, 2017

Uh oh!

refack commented Sep 2, 2017

Uh oh!

ian-perkins commented Sep 2, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

maclover7 Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Sep 3, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

maclover7 commented Aug 24, 2017 •

edited by refack

Loading

Trott commented Sep 2, 2017 •

edited

Loading

refack commented Sep 2, 2017 •

edited

Loading

Trott commented Sep 4, 2017 •

edited

Loading

maclover7 commented Sep 8, 2017 •

edited

Loading