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

Replacing benchmark/compare.R with a JavaScript implementation #45390

Open
joyeecheung opened this issue Nov 9, 2022 · 1 comment
Open

Replacing benchmark/compare.R with a JavaScript implementation #45390

joyeecheung opened this issue Nov 9, 2022 · 1 comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. meta Issues and PRs related to the general management of the project.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Nov 9, 2022

Since I updated to macOS Monterey and R 4.2+ I am no longer able to install the packages required by compare.R. I recently found out about https://github.com/targos/node-benchmark-compare (h/t @targos) and gave it a try, it worked very well for me. I think in the past we had discussion about replacing this script with a JS implementation, it didn't end up fruitful because of the lack of implementation of the statistical distribution required by the script. Now that seems to be solved already by https://github.com/nearform/node-cephes (see #44974 (comment)). So I am wondering if it's time to reopen the discussion.

I think we can basically just vendor in the node-benchmark-compare package into tools/ or benchmark/ and replace the command cat results.csv | Rscript benchmark/compare.R with node-benchmark-compare results.csv (both the benchmark guide and the script used by the CI need to be updated, though in the case of the latter we might have to un-archive the repo, or edit the CI config to point to somewhere else?)

@joyeecheung joyeecheung added meta Issues and PRs related to the general management of the project. benchmark Issues and PRs related to the benchmark subsystem. labels Nov 9, 2022
@Trott
Copy link
Member

Trott commented Nov 10, 2022

Since node-benchmark-compare is already described in the benchmark guide at https://github.com/nodejs/node/blob/main/doc/contributing/writing-and-running-benchmarks.md#benchmark-analysis-requirements and https://github.com/nodejs/node/blob/main/doc/contributing/writing-and-running-benchmarks.md#filtering-benchmarks, the changes there could be small. Like, you could move the node-benchmark-compare stuff above the compare.R stuff in the benchmark guide, and maybe that's all we need to do.

IThe scripts that need to be updated are...in @addaleax's fork of that repo. 😱 We probably want to change that....

image

Anyway, this is a fork of the repo you linkied to that was archived. The script still uses compare.R. So I guess we'd have to change it to use npx node-benchmark-compare or go with your plan of putting node-benchmark-compare in the core repo. (If we do that, let's perhaps grab run.sh from Anna's fork and put it here or in the build repo instead?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

2 participants