chore: switch performance tests to hyperfine #19919
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
There was a problem hiding this comment.
LGTM. Tested locally and works like a charm. Would like @mdjermanovic to review before merging.
Makefile.js
Outdated
|
|
||
| results.sort((a, b) => a - b); | ||
| const median = results[~~(results.length / 2)]; | ||
| const loadingCommand = `"${process.execPath}" --require "${require("./package.json").main}" ""`; |
There was a problem hiding this comment.
Question for better understanding: what is the expected purpose of the "" at the end of the command? When I run the command ("C:\Program Files\nodejs\node.exe" --require "./lib/api.js" "") directly, Node.js prompts for input. Perhaps it was intended to be --eval ""? Though, I don't see any difference between "" present, "" omitted, and --eval "" when running npm run test:performance.
There was a problem hiding this comment.
You are right, it seems that "" is actually unnecessary. When I run node --require "./lib/api.js" "" I also see Node.js running in REPL mode and prompting for input. I imagine that hyperfine is suppressing stdin and so the command behaves like echo "" | node --require "./lib/api.js" "", and that explains the different behavior. If this difference is surprising we could instead add --eval=0 or alternatively just do `"${process.execPath}" "${require("./package.json").main}"`, to require ./lib/api.js as an entry point.
There was a problem hiding this comment.
just do
`"${process.execPath}" "${require("./package.json").main}"`, to require./lib/api.jsas an entry point.
This looks good to me 👍
There was a problem hiding this comment.
Thanks! Updated in c50f3a6.
I'm getting very similar results as with the old (current) "Single File" and "Multi Files" performance tests implementation. But, unlike with the old implementation, results with the new implementation seem to be very consistent 👍 |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:
Update performance tests
Fixes #19680
What changes did you make? (Give an overview)
Updated performance tests to use hyperfine.
./lib/api.js. This is why it reports larger values than the previously.Example output of
npm run test:performance:Is there anything you'd like reviewers to focus on?
I haven't changed the benchmark commands themselves, so for example the "Single File" test still measures the time to lint
tests/performance/jshint.jswith all built-in rules turned on; "Multi Files" downloads and lints ESLint v1.10.3. For this reason it should be possible to directly compare the new hyperfine-generated results with the results generated by the current implementation, at least for these two tests. Ideally, hyperfine should generate less variable, hence more consistent results across runs.