Skip to content

benchmark: test refactoring - #31396

Closed
BridgeAR wants to merge 4 commits into
nodejs:masterfrom
BridgeAR:2020-01-17-benchmark-test-refactoring
Closed

benchmark: test refactoring#31396
BridgeAR wants to merge 4 commits into
nodejs:masterfrom
BridgeAR:2020-01-17-benchmark-test-refactoring

Conversation

@BridgeAR

@BridgeARBridgeAR commented Jan 17, 2020•
edited
Loading

Copy link
Copy Markdown
Member

I went ahead and refactored all our benchmark tests. The benchmark suite itself now has a test functionality built-in (using test). It is also possible to run all benchmark suites by using all. The tests are now run independent from benchmark changes and should always keep on working.

The fixtures directory is now skipped and in case there's a typo for the benchmark, the error message is now more informative.

I removed warnings from benchmark files in case they occurred.

Some tests require special options as a minimum. Those may now be passed through using the test option. These values will be used in case the benchmark test is run.

It is now possible to pass through individual configurations instead of having to use an array.

Fixes: #31083

I did not yet find time to clean up the commits.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR
BridgeAR requested a review from TrottJanuary 17, 2020 08:03
@nodejs-github-botnodejs-github-bot added assert Issues and PRs related to the assert subsystem. async_hooks Issues and PRs related to the async hooks subsystem. benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to the fs subsystem / file system. http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem. labels Jan 17, 2020
mscdex
mscdex reviewed Jan 17, 2020
View reviewed changes
Comment threadbenchmark/_test-double-benchmarker.js Outdated
BridgeAR
BridgeAR commented Jan 17, 2020
View reviewed changes
Comment threadbenchmark/_test-double-benchmarker.js Outdated

@BridgeARBridgeARJan 17, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@nodejs/http2 @ronag I had to end the process hard in case this runs the http2 benchmarks. Maybe someone could have a look what part stays alive.

@BridgeAR
BridgeARforce-pushed the 2020-01-17-benchmark-test-refactoring branch 2 times, most recently from 1cb7b50 to 27eee27CompareJanuary 20, 2020 10:22
@BridgeAR
BridgeAR marked this pull request as ready for review January 20, 2020 10:25
@BridgeAR

BridgeAR commented Jan 20, 2020

Copy link
Copy Markdown
MemberAuthor

This is ready for review. I just cleaned up the commits.

sam-github
sam-github approved these changes Jan 20, 2020
View reviewed changes

@sam-githubsam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks reasonable to me.

@BridgeAR

BridgeAR commented Jan 20, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/11672/ ✔️

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2020
@BridgeAR

BridgeAR commented Jan 20, 2020

Copy link
Copy Markdown
MemberAuthor

This does reduce the test runtime for almost all benchmarks. Mostly by a few hundred milliseconds. The runtime for test-benchmark-async-hooks is reduced more significantly from ~6 to ~1 seconds and test-benchmark-module from ~3 to ~1 seconds.

@BridgeAR

BridgeAR commented Jan 22, 2020

Copy link
Copy Markdown
MemberAuthor

This could use another review @nodejs/benchmarking

@BridgeAR
BridgeARforce-pushed the 2020-01-17-benchmark-test-refactoring branch from 27eee27 to 3fd40c6CompareJanuary 25, 2020 09:47
@Trott

Trott commented Jan 27, 2020

Copy link
Copy Markdown
Member

Needs a rebase.

Trott
Trott approved these changes Jan 27, 2020
View reviewed changes

@TrottTrott left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rubber-stamp LGTM. The concept is sound.

@BridgeAR
BridgeARforce-pushed the 2020-01-17-benchmark-test-refactoring branch from 3fd40c6 to 6ea89f5CompareFebruary 5, 2020 13:20
@BridgeAR

BridgeAR commented Feb 6, 2020

Copy link
Copy Markdown
MemberAuthor

CI https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/12025/

@BridgeAR
BridgeARforce-pushed the 2020-01-17-benchmark-test-refactoring branch 2 times, most recently from 9bdb710 to f7926f3CompareFebruary 8, 2020 22:40
@BridgeAR

BridgeAR commented Feb 8, 2020

Copy link
Copy Markdown
MemberAuthor

CI https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/12097/

@BridgeAR
benchmark: refactor helper into a class
1e17257
This is the basis to refactor the helper to use modern class
language features such as private fields.
It also refactors the exports to use module.exports. That way it's
immediately clear what parts are exported.
addaleax added a commit that referenced this pull request Feb 10, 2020
@addaleax
Revert "benchmark: refactor helper into a class"
5b0308c
This reverts commit b70741e.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mscdex

mscdex commented Feb 11, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Also something that I just now noticed is that this PR changed the formatting of the configuration string (when writing the CSV output) such that each parameter is preceded by a space whether it's needed or not, which breaks tooling (something I'm currently working on) that monitors changes in benchmark results by comparing results for a particular configuration.

Just something to keep in mind if these changes get merged again later on.

EDIT: FWIW I'm now incorporating extra safeguards to prevent this sort of thing in my tooling in the future...

@BridgeARBridgeAR mentioned this pull request Feb 12, 2020
Closed
4 tasks
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@BridgeAR@codebytere
benchmark: refactor helper into a class
9f2c742
This is the basis to refactor the helper to use modern class
language features such as private fields.
It also refactors the exports to use module.exports. That way it's
immediately clear what parts are exported.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@BridgeAR@codebytere
benchmark: add test and all options and improve errors
ca343ca
This adds a new `test` option. Using it automatically uses a single
minimal option matrix to verify the benchmark works as expected.
Using the new `all` option makes sure all test suites are run.
On top of that the benchmarks will from now on report properly
what category might have a typo, if any.
The http duration was also refactored to use a option instead of
relying on a configuration setting.
The fixture folder is ignored as test suite from now on.
PR-URL: #31396Fixes: #31083
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@BridgeAR@codebytere
test: refactor all benchmark tests to use the new test option
4878c7a
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@BridgeAR@codebytere
benchmark: remove special test entries
1484f5a
It was necessary to have fallbacks to run the original tests. This
is obsolete with the new test mode.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax@codebytere
Revert "benchmark: remove special test entries"
30f55ce
This reverts commit 357230f.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax@codebytere
Revert "test: refactor all benchmark tests to use the new test option"
5b3c4b3
This reverts commit 78aa348.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax@codebytere
Revert "benchmark: add test and all options and improve errors"
8974fa7
This reverts commit dac5795.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax@codebytere
Revert "benchmark: refactor helper into a class"
819fb76
This reverts commit b70741e.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@codebyterecodebytere mentioned this pull request Feb 17, 2020
Merged
@codebytere

codebytere commented Mar 15, 2020

Copy link
Copy Markdown
Member

@BridgeAR could you please manually backport this to v12.x?

codebytere pushed a commit that referenced this pull request Mar 15, 2020
@BridgeAR@codebytere
benchmark: refactor helper into a class
fc97aa5
This is the basis to refactor the helper to use modern class
language features such as private fields.
It also refactors the exports to use module.exports. That way it's
immediately clear what parts are exported.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@BridgeAR@codebytere
benchmark: remove special test entries
9e6d3c4
It was necessary to have fallbacks to run the original tests. This
is obsolete with the new test mode.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@BridgeAR@codebytere
benchmark: refactor helper into a class
0b37261
This is the basis to refactor the helper to use modern class
language features such as private fields.
It also refactors the exports to use module.exports. That way it's
immediately clear what parts are exported.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@BridgeAR@codebytere
benchmark: remove special test entries
6b74b10
It was necessary to have fallbacks to run the original tests. This
is obsolete with the new test mode.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@BridgeAR@codebytere
benchmark: refactor helper into a class
19fbe55
This is the basis to refactor the helper to use modern class
language features such as private fields.
It also refactors the exports to use module.exports. That way it's
immediately clear what parts are exported.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@BridgeAR@codebytere
benchmark: remove special test entries
c18cec7
It was necessary to have fallbacks to run the original tests. This
is obsolete with the new test mode.
PR-URL: #31396
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax@targos
Revert "benchmark: remove special test entries"
66c5f50
This reverts commit 357230f.
Refs: nodejs#31396
PR-URL: nodejs#31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax@targos
Revert "benchmark: refactor helper into a class"
4b07730
This reverts commit b70741e.
Refs: nodejs#31396
PR-URL: nodejs#31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax@targos
Revert "benchmark: remove special test entries"
ef80c02
This reverts commit 357230f.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax@targos
Revert "benchmark: refactor helper into a class"
cf2ca11
This reverts commit b70741e.
Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
+2 more reviewers
@mscdexmscdexmscdex left review comments
@sam-githubsam-githubsam-github approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

assertIssues and PRs related to the assert subsystem.async_hooksIssues and PRs related to the async hooks subsystem.benchmarkIssues and PRs related to the benchmark subsystem.bufferIssues and PRs related to the buffer subsystem.cryptoIssues and PRs related to the crypto subsystem.fsIssues and PRs related to the fs subsystem / file system.httpIssues or PRs related to the http subsystem.http2Issues or PRs related to the http2 subsystem.netIssues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Benchmarks should expose their options

8 participants

@BridgeAR@Trott@addaleax@mscdex@codebytere@sam-github@targos@nodejs-github-bot