-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
numberOfProcessorCores() computes the core count once and caches it. Bun needs the same count read fresh on every call for os.availableParallelism(), so that a change to the affinity mask or to the cgroup cpu quota made after startup is observed, as in libuv's uv_available_parallelism(). Move the platform body into numberOfProcessorCoresUncached() and make numberOfProcessorCores() cache its result. The env override and the parse warning keep their behaviour. The warning prints once.
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 — mechanical extract-function refactor with no behavior change to the cached path.
What was reviewed:
- Each
OS(...)branch:s_numberOfCores = X→return Xis 1:1, no branch left without a return, and the fallback#elsestill returnsdefaultIfUnavailable. - The cached wrapper:
numberOfProcessorCoresUncached()always returns ≥ 1, so thes_numberOfCores > 0guard still works and the first-call semantics are unchanged. - The once-only warning:
static bool+std::exchangematches the thread-safety level of the pre-existings_numberOfCoresstatic; worst case is a duplicate stderr line.
Extended reasoning...
Overview
This PR touches two files in WTF: Source/WTF/wtf/NumberOfCores.cpp and its header. It extracts the platform-specific body of numberOfProcessorCores() into a new exported numberOfProcessorCoresUncached() that queries the OS on every call, and rewrites numberOfProcessorCores() as a thin caching wrapper around it. The only non-mechanical additions are a static bool guard so the unparseable-env-var warning prints once even under repeated uncached calls, and an #include <utility> for std::exchange. The header gains a declaration and doc comment for the new function.
Security risks
None. The function reads processor-count information from the OS (sysctl, sysconf, sched_getaffinity, GetSystemInfo) and an environment variable that was already read before this change. No new inputs, no auth/crypto/permissions surface, no user-controlled data beyond the pre-existing env-var override which is already clamped to [1, 1024].
Level of scrutiny
Low. This is a textbook extract-function refactor: every s_numberOfCores = <expr> assignment in the per-OS #if ladder became return <expr> with the expression untouched, and the caching wrapper reproduces the original static int s_numberOfCores = -1; if (> 0) return; ... return pattern verbatim. I checked each branch (DARWIN, LINUX, AIX/BSD/HAIKU, QNX, WINDOWS, and the #else fallback) and confirmed none was dropped or altered. The uncached function always returns at least 1, so the > 0 cache guard cannot loop or re-query. The s_warnedAboutEnv static is a plain bool rather than atomic, but that matches the existing non-atomic s_numberOfCores static in the same function — the codebase already accepts that race, and the failure mode is a harmless duplicate warning line.
Other factors
No CODEOWNERS entry covers Source/WTF/, so no owner sign-off is required for these paths. The PR timeline shows no prior reviews or outstanding objections. The change is small (~40 lines net), self-contained to one translation unit plus its header, and the intent (letting Bun's os.availableParallelism() re-read affinity/cgroup limits without duplicating the Linux logic) is clearly stated and matches the code. The bug-hunting run exited on dry_streak with no findings.
Preview Builds
|
Problem
WTF::numberOfProcessorCores()computes the core count once and caches it in a static. Bun reports it fromos.availableParallelism(). Node evaluatesuv_available_parallelism()on every call, so atasksetaffinity change or a cgroupcpu.maxresize made after startup is observed there and not in Bun.NumberOfCores.cppand of the privateuv_get_constrained_cpu(). The fork is where this logic lives (JSC stress test stack-overflow-in-syntax-checker.js is flaky. WebKit/WebKit#28801 in bun).Fix
numberOfProcessorCores()into a new exportednumberOfProcessorCoresUncached().numberOfProcessorCores()now caches its result. No platform branch changes.WTF_numberOfProcessorCoresoverride (and the[1, 1024]clamp from WTF: clamp the WTF_numberOfProcessorCores override to [1, 1024] #571 ) applies to both. The parse warning prints once per process, so a caller that reads the uncached value in a loop does not flood stderr.numberOfProcessorCoresUncached()fromos.availableParallelism()and pinsWEBKIT_VERSIONto this branch.12 2aftertaskset -pc 40-41 <pid>(was12 12).navigator.hardwareConcurrencyand JSC's thread sizing keep the cached value.