Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

benchmark: update README to include all benchmarks #46991

Merged

Conversation

Theo-Steiner
Copy link
Contributor

@Theo-Steiner Theo-Steiner commented Mar 7, 2023

Background

During #46731 @cola119 mentioned that the benchmark for readline was not documented in benchmark/README.md.
This PR will fix this (and also add other unmentioned benchmarks).

Update the markdown table in benchmark/README.md

The content of benchmark/README.md has become stale, no longer providing an overview over all existing benchmarks. To address this, the markdown table has been updated to incorporate all current benchmarks.

Move benchmarks for parts of submodules into the corresponding directory

Furthermore, some benchmarks were in separate directories from the benchmarks of their respective node submodules. These benchmarks have been moved into the directory of their submodule.

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem. module Issues and PRs related to the module subsystem. labels Mar 7, 2023
@aduh95
Copy link
Contributor

aduh95 commented Mar 7, 2023

Since it looks like we are not doing a good job at maintaining that table, what if we removed it? I don't think it's very useful, and without tests it seems inevitable that it will one day or another get back to an outdated state.

@cola119
Copy link
Member

cola119 commented Mar 7, 2023

As a beginner in benchmarking, I find the table not very helpful, and I believe that documents describing the benchmark directory structure would be more useful.

@anonrig
Copy link
Member

anonrig commented Mar 7, 2023

Since it looks like we are not doing a good job at maintaining that table, what if we removed it? I don't think it's very useful, and without tests it seems inevitable that it will one day or another get back to an outdated state.

I agree with @aduh95 here. This is the first time I saw this document :-)

@Theo-Steiner
Copy link
Contributor Author

I've reverted the previous changes and included a description of the directory structure in place of the outdated table.

benchmark/README.md Outdated Show resolved Hide resolved
@cola119 cola119 removed buffer Issues and PRs related to the buffer subsystem. module Issues and PRs related to the module subsystem. labels Mar 8, 2023
benchmark/README.md Outdated Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Mar 8, 2023

Can you update the initial commit title & pull-request title to a more relevant one @Theo-Steiner? commit-queue-squash uses the first commit title.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

LGTM when first commit title is changed to a more relevant title for the current changes.

@Theo-Steiner Theo-Steiner force-pushed the docs/update-benchmarking-readme branch from 29f2380 to 101dc0a Compare March 8, 2023 13:34
The markdown table in `benchmark/README.md` has grown stale, no
longer providing an overview over all existing benchmarks. As it has
proven difficult to keep an exhaustive listing of available benchmarks
up to date, this commit provides a description of how the directory is
structured instead.
@Theo-Steiner Theo-Steiner force-pushed the docs/update-benchmarking-readme branch from 101dc0a to 182abbe Compare March 9, 2023 00:24
@cola119 cola119 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 9, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit 10c1ab0 into nodejs:main Mar 11, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 10c1ab0

targos pushed a commit that referenced this pull request Mar 13, 2023
The markdown table in `benchmark/README.md` has grown stale, no
longer providing an overview over all existing benchmarks. As it has
proven difficult to keep an exhaustive listing of available benchmarks
up to date, this commit provides a description of how the directory is
structured instead.

PR-URL: #46991
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
The markdown table in `benchmark/README.md` has grown stale, no
longer providing an overview over all existing benchmarks. As it has
proven difficult to keep an exhaustive listing of available benchmarks
up to date, this commit provides a description of how the directory is
structured instead.

PR-URL: #46991
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
The markdown table in `benchmark/README.md` has grown stale, no
longer providing an overview over all existing benchmarks. As it has
proven difficult to keep an exhaustive listing of available benchmarks
up to date, this commit provides a description of how the directory is
structured instead.

PR-URL: #46991
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants