Simplify cpuCount utility and add limiting functionality #5673
Conversation
|
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
|
I'll have to measure what the best number on an 4 core i5 without hyperthreading is.... |
DeMoorJasper
force-pushed
the
simplify-cpu-count
branch
from
850bf81
to
cea9ee3
Compare
|
@mischnic have you been able to test this yet? |
DeMoorJasper
requested review from
mischnic,
devongovett and
wbinnssmith
and removed request for
devongovett
|
Windows 10, i5-4570 @ 3.20GHz (4 cores, 4 threads) PARCEL_WORKERS: time |
|
@mischnic So (cores - 1) for real cores and (threads / 2) when multithreaded or should it be (threads / 2 + 1). Guess we can't really test the last idea unless someone has a 6 or 8 core processor device to test this on? |
|
(I only did a single run, no averaging) Honestly not sure if there's a benefit to changing the detection logic at all. |
|
Originally I expected it to have a larger performance impact. But I wrote it anyway so created a PR, however I still see value in creating a simpler version that works regardless of os. |
|
What happened to this PR? This does affect memory usage. Should we merge the following instead? |
|
@aminya I just closed it because it got kinda old without any approval. I reopened it in case anyone wants to merge it |
| let cachedCoreCount = 0; | ||
| const platform = os.platform(); | ||
| export function getCoreCount( | ||
| limit: number = platform === 'win32' ? 4 : 8, |
There was a problem hiding this comment.
This is not an OS-relevant issue. The parcel doesn't use the threads it spawns. Why do you want to create so many threads which Parcel doesn't use?
Other than workers 0 and 1, the others almost do nothing.
There was a problem hiding this comment.
I thought this was a filesystem limitation of windows
There was a problem hiding this comment.
| limit: number = platform === 'win32' ? 4 : 8, | |
| limit: number = 4, |
There was a problem hiding this comment.
The smarter way is to spawn threads proportional to the work that Parcel wants to do. For example:
min((fileNumbers)/20, coresAvailable)So, if there are 100 files and we have 16 cores, it will spawn only 5 cores.
There was a problem hiding this comment.
The smarter way is to spawn threads proportional to the work that Parcel wants to do. For example:
Yes, but we don't (always) know this beforehand.
There was a problem hiding this comment.
We should use the information when it is available. When not known, we can just use a constant:
min(fileNumbers ? (fileNumbers)/20 : 4, coresAvailable)There was a problem hiding this comment.
What is fileNumbers?
On an initial build (where there's the most work), we don't know.
For subsequent builds, we only know the filecount of the previous build, not how much changed (= how much work there is).
So I would say this isn't possible to know at all
There was a problem hiding this comment.
So maybe it is better to perform an initial analysis using all threads. Then after that, it can use a suitable number of workers for the actual build.
There was a problem hiding this comment.
That would probably slow it down more than it would speed up?
↪️ Pull Request
This PR simplifies the cpuCount util to not check for real cores and just always use a maximal of half the threads that are available.
This shaves of 6ms startup time but mainly simplifies the logic a ton, making it more reliable across platforms.
Also added a limit parameter, that is set to 8 for now a that seems to be plenty for now (until we figure out which amount is perfect)