Conversation
nodejs-github-bot
added
benchmark
|
Why don't we pass the function to benchmakr as a parameter to createBenchmark() like and later access it as or something which resolves into |
|
That requires changing all benchmarks and this is subject to developer errors. I tried to do it harmlessly (funny saying that while I'm monkey patching Module.require). But, possibly simpler than my approach. |
We can add an eslint rule (possibly @aduh95) to call that parameter inside |
This comment was marked as spam.
This comment was marked as spam.
|
This pull request has been marked as stale due to 90 days of inactivity. |
github-actions
Bot
added
the
stale
5 participants
Hey,
I'm opening it as a draft as there are a lot of loose ends to solve before considering such usage. This is an experiment related to research I'm conducting.
The idea is to generate a benchmark coverage for all of our exported modules. For instance, check if the functions exposed by
require('node:fs')are covered in our benchmark suite (benchmark/fs/*.js). To achieve an intermediary goal I had to monkey-patch theModule.prototype.requirebefore executing each benchmark (we run each one in a separate process) to return a singleton that basically manages the state of how many times that function was called.I have tried to use our
test_runnercoverage for that, but it couldn't identify the location of built-in modules (require('node:*')) which is expected by the nature of a coverage tool. The other reason it didn't fit was the need to cover only the exported modules, not thelib/internal/*functions. The result I wanted to have was: "I need to know which functions/classes that are exposed to users do not contain a benchmark".It's also worth it to mention that, a nested call is ignored in the benchmark report. Example:
fs.existscallsfs.accessbehind the scenesnode/lib/fs.js
Line 260 in b8a2550
fs.existsandfs.accessas "covered" in the benchmark report. It should only countfs.existsas covered.benchmark/**/*.jsResults
Details
Full result: https://gist.github.com/RafaelGSS/66d33091560d61932b26d74af2fa8b82
Current limitations
coverage.jsto set up the monkey patch ofModule.require(-r ./coverage.js). However, any benchmark setup would impact the report. For instance:This will result in:
fs.existsSyncstatus "covered"fs.readdirstatus "covered"This is unfortunate, as in reality only
fs.existsSyncis being measured. One theory that came to my mind to fix that is to intercept thebench.startcalls and only then return the patched modules. I might try it later.typeof require('node:assert') === 'function'. This also means that classes such asAsyncLocalStorage(or function classes) aren't covered as the constructor (called withnew ..) isn't patched. I think usingProxymight solve it, but I haven't tested yet.I'm certain there's a better way to approach this goal and fix the limitations. Hence, I'm opening it as a draft.
cc: @nodejs/benchmarking @nodejs/test_runner @lemire