-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
perf: improve worker count calculation for "auto"
concurrency
#20067
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
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
357e07f
to
4230d0b
Compare
//----------------------------------------------------------------------------- | ||
|
||
module.exports = { | ||
createDebug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the debug
utility's main export, but it makes sure that the %t
formatter is registered. I'm using this to format differences of high-resolution timer values (process.hrtime.bigint()
). I'm not sure if this should be made more explicit with a different name, a comment, etc.
tests/lib/eslint/eslint.js
Outdated
|
||
describe("caclulateWorkerCount", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved these tests here so they will be repeated with and without the v10_config_lookup_from_file
flag. This is because calculateWorkerCount
now uses the config loader.
const fCache = require("file-entry-cache"); | ||
const sinon = require("sinon"); | ||
const proxyquire = require("proxyquire").noCallThru().noPreserveCache(); | ||
const proxyquire = require("proxyquire").noCallThru(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing test failures because of ESLint
objects being created from a mocked version of lib/eslint/eslint.js
. With noPreserveCache
set, a mocked version of the lib/eslint/eslint.js
module was being kept in the require
cache, with its own version of privateMembers
, and that did no longer match the privateMembers
in the scope of the caclulateWorkerCount
function, which is imported from the original module.
{ WarningService } = require("../../lib/services/warning-service"); | ||
|
||
const proxyquire = require("proxyquire").noCallThru().noPreserveCache(); | ||
const proxyquire = require("proxyquire").noCallThru(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this was causing inconsistencies in Node.js' require
cache, resulting in surprising test failures.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor suggestions, then LGTM.
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Would like @nzakas to review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just waiting for the patch release window to close.
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:
Improve worker count calculation for
"auto"
concurrency.refs #20040 (comment)
What changes did you make? (Give an overview)
This pull request makes some improvements to the thread count calculation that is done when the concurrency is
"auto"
.AUTO_FILES_PER_WORKER
constant from 35 to 50. This constant value affects the thread count calculation when only a few files are linted. The most visible effect is that multithreading will be no longer turned on when linting less than 51 files (this is an empiric value and possibly still too low on some machines). I realized that some of the projects I used in my early tests were biased by ignored files. After repeating the tests with ignored files not counted, the larger value seems more appropriate.Is there anything you'd like reviewers to focus on?
The additional time to count files that need processing (excluding ignored and cached files) is printed in a debug line, for example:
eslint:eslint 351 file(s) to process counted in 18 ms +19ms
In practice, the effective overhead seems to be even lower, possibly because of cross-thread caching by the OS, which results in subsequent access to file metadata being faster.
Benchmarks
I tested the implementation on a fork of the OpenUI5 repo which contains 12,500 lintable files and uses the
--cache
option: https://github.com/mauriciolauffer/openui5/tree/eslint-v9.These are the results when the cache file is outdated:
It takes just 3 milliseconds to determine that the maximum number of threads (i.e. 4 for an 8 core CPU) should be used.
Now, here are the results when the cache file is present and all cached results are valid:
In this case, it takes 56 milliseconds to determine that no concurrency should be used. The process is slower compared to the previous run because a much larger number of file metadata is checked, until it is determined that there aren't enough files to process for multithread mode.
I also repeated the same tests on a slower Windows machine with 16 cores, with comparable results:
No files cached
All files cached
As already mentioned, the actual time overhead seems to be even lower than the figures suggests because of the reduced time for other operation, to the extent that there is no noticeable difference between
--concurrency=auto
and--concurrency=off
when most of the files are cached: