GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — benchmark: test refactoring by BridgeAR · Pull Request #31396 · nodejs/node

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Jan 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@BridgeAR
BridgeAR requested a review from Trott January 17, 2020 08:03
@nodejs-github-bot nodejs-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 Node.js benchmarks and benchmarking infrastructure. buffer Issues and PRs related to the buffer subsystem. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to file-system APIs and the fs module. http Issues and PRs related to the http subsystem. http2 Issues and 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 thread benchmark/_test-double-benchmarker.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR
BridgeAR commented Jan 17, 2020
View reviewed changes
Comment thread benchmark/_test-double-benchmarker.js Outdated

@BridgeAR BridgeAR Jan 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@BridgeAR
BridgeAR force-pushed the 2020-01-17-benchmark-test-refactoring branch 2 times, most recently from 1cb7b50 to 27eee27 Compare January 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
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

@sam-github sam-github left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks reasonable to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@BridgeAR

BridgeAR commented Jan 20, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 20, 2020
@BridgeAR

BridgeAR commented Jan 20, 2020

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BridgeAR

BridgeAR commented Jan 22, 2020

Copy link
Copy Markdown
Member Author

This could use another review @nodejs/benchmarking

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BridgeAR
BridgeAR force-pushed the 2020-01-17-benchmark-test-refactoring branch from 27eee27 to 3fd40c6 Compare January 25, 2020 09:47
@Trott

Trott commented Jan 27, 2020

Copy link
Copy Markdown
Member

Needs a rebase.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

@Trott Trott left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@BridgeAR
BridgeAR force-pushed the 2020-01-17-benchmark-test-refactoring branch from 3fd40c6 to 6ea89f5 Compare February 5, 2020 13:20
@BridgeAR

BridgeAR commented Feb 6, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

BridgeAR commented Feb 8, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mscdex

mscdex commented Feb 11, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BridgeAR BridgeAR mentioned this pull request Feb 12, 2020
benchmark: reland test refactoring #31755
Closed
4 tasks done
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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: #31396
Fixes: #31083
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
v13.9.0 proposal #31837
Merged
@codebytere

codebytere commented Mar 15, 2020

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@codebytere codebytere added backport-requested-v12.x and removed backport-requested-v12.x labels Mar 15, 2020
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
v12.16.2 proposal #32313
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos removed author ready PRs with CI started, the required approvals, and no outstanding review comments. 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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos added the dont-land-on-v12.x label Apr 25, 2020
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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 Node.js benchmarks and benchmarking infrastructure. buffer Issues and PRs related to the buffer subsystem. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to file-system APIs and the fs module. http Issues and PRs related to the http subsystem. http2 Issues and PRs related to the http2 subsystem. net Issues 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

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@BridgeAR @Trott @addaleax @mscdex @codebytere @sam-github @targos @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.