Skip to content

lib/*.js (benchmark): expose benchmark functionality as a module - #44974

Closed
zombieleet wants to merge 7 commits into
nodejs:mainfrom
zombieleet:benchmark-module
Closed

lib/*.js (benchmark): expose benchmark functionality as a module#44974
zombieleet wants to merge 7 commits into
nodejs:mainfrom
zombieleet:benchmark-module

Conversation

@zombieleet

@zombieleet zombieleet commented Oct 12, 2022

Copy link
Copy Markdown
Contributor

Draft PR to expose the benchmark functionality as part of the core nodejs module.

It's still WIP

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. errors Issues and PRs related to JavaScript errors originating in Node.js core. needs-ci PRs that need a full CI run. labels Oct 12, 2022
@zombieleet zombieleet changed the title Benchmark module lib/*.js (benchmark): expose benchmark functionality as a module Oct 12, 2022
@Trott

Trott commented Oct 12, 2022

Copy link
Copy Markdown
Member

@nodejs/benchmarking @nodejs/tsc

I'm not sure our current benchmark implementation is something that we'd want to expose, but I'm also not sure it isn't. I've always imagined it as kind of ragged and for our own use. But maybe I don't give it enough credit.

remove benchmark.js from test/common to test/benchmark
change import part in all test-benchmark-* file

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

I don't have an opinion whether this should be exposed or not, however in order to land there are a couples of things that need to change on this PR:

  • Please remove from lib/ the files whose names starts with an underscore, and move them to lib/internal/benchmark instead.
  • Please make sure that only the new module can only be loaded with the node: prefix (require('node:benchmark')). require('benchmark') should still refer to userland (e.g. node_modules/benchmark/index.js). To achieve that, add it to that set:
    // Modules that can only be imported via the node: scheme.
    const schemelessBlockList = new SafeSet([
    'test',
    ]);
  • This needs documentation.

@mscdex

mscdex commented Oct 12, 2022

Copy link
Copy Markdown
Contributor

I agree with @Trott, the existing benchmarking system is not something we should just dump into a public API. If we decide node core needs to be exposing this sort of functionality, we should instead create something that's designed to be more user/developer-friendly (along with real unit tests) since the existing system was designed more around node's own needs.

One of the big issues with doing benchmarking in an environment like this is getting the statistical side right, using javascript. I know there have been attempts at this, but AFAIK using R to do the calculations is still the most reliable way of doing so, so we would need someone familiar enough with R to be able to convert the existing benchmark/*.R scripts safely to JS. I think part of the solution for this would include utilizing BigInts to simulate a "BigDecimal" type for the number crunching in order to maintain precision and accuracy.

@targos

targos commented Oct 12, 2022

Copy link
Copy Markdown
Member

I know there have been attempts at this, but AFAIK using R to do the calculations is still the most reliable way of doing so, so we would need someone familiar enough with R to be able to convert the existing benchmark/*.R scripts safely to JS.

I did that in https://github.com/targos/node-benchmark-compare. The JS version gives the exact same numeric results as the R one. The issue isn't really the language (R also uses IEEE 754 floating point numbers), but that we need to be able to compute some statistical distributions. My package ends up using https://github.com/nearform/node-cephes (WASM)

@zombieleet

Copy link
Copy Markdown
Contributor Author

@mscdex @Trott
The existing implementation is actually okay but not user/developer friendly (some parts of it also makes use of BigInt), I just need to make it more user/developer friendly, if it needs to be rewritten, I am fine doing that as well.

@anonrig

anonrig commented Oct 13, 2022

Copy link
Copy Markdown
Member

Is there an open issue to discuss the changes stated in this pull request?

@zombieleet

Copy link
Copy Markdown
Contributor Author

Is there an open issue to discuss the changes stated in this pull request?

@anonrig sorry for the late reply, here is an issue I created #45037

implementation of benchmark/compare.js is now part of lib/benchmark.js
add validaions for http options as well as normal benchmark options
add few jsdoc comments
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 Node.js benchmarks and benchmarking infrastructure. errors Issues and PRs related to JavaScript errors originating in Node.js core. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants