Clamp GetThreadLimit by the live OpenMP thread limit - #1224
Conversation
OpenFHEParallelControls::GetThreadLimit(n) returned min(n, machineThreads), where machineThreads is latched once from omp_get_max_threads() at static init and never refreshed. Every parallel hot region emits num_threads(GetThreadLimit(...)), and an explicit num_threads clause overrides the runtime OpenMP ICV -- so a post-init omp_set_num_threads(), including one made through OpenFHEParallelControls::SetNumThreads(), is silently ignored and the library cannot be capped to fewer threads in-process. Also clamp by the live omp_get_max_threads() so a runtime thread limit is honored on every platform. The added call is a cheap ICV read; when the runtime limit is left at the machine default the result is unchanged.
d4ae500 to
a14924a
Compare
|
Rebased onto |
|
Thanks for the detailed report — your diagnosis of the mechanism is exactly right: the explicit num_threads(GetThreadLimit(...)) clause overrides the nthreads ICV, so runtime caps never reach the hot regions. I reviewed your proposed fix, and there's a case it doesn't fully cover: omp_get_max_threads() reads the calling thread's ICV, which is thread-local. If an application sets the cap on its main thread but runs FHE work from a worker/pool thread, the live read on that worker still returns the machine default and the cap is ignored again. You can verify this by calling omp_set_num_threads(2) on the main thread and reading omp_get_max_threads() from a fresh std::thread — it reports the machine default. I've opened PR #1233 with a different approach: OpenFHEParallelControls now keeps a process-global, atomically-read thread limit that SetNumThreads()/Enable()/Disable() update and every GetThreadLimit() clause consults, regardless of which thread enters the region — at zero per-region overhead. It also adds num_threads clauses to the regions that had none (matrix, matrixstrassen, dgsampling, CKKS scheme switching) and fixes several adjacent problems (Disable() being bypassed the same way, UnitTestStart()/UnitTestStop() clobbering caps, omp_set_num_threads(0) being reachable). Please review and confirm it resolves your use case. One usage note: a bare omp_set_num_threads() at the application layer is still not sufficient to cap the library — the explicit num_threads clauses override the ICV per the OpenMP spec, and the ICV is per-thread besides. OpenFHEParallelControls.SetNumThreads() is the supported way to cap OpenFHE; it applies process-wide from any thread. |
|
One workflow request for future contributions: your PR came from a fork of openfhe-development, which meant I couldn't push additional commits or cherry-pick your changes into the PR branch directly. Next time, please create a branch in the openfhe-development repo itself rather than working from a fork — that lets us collaborate on the PR branch and add fixes on top of your commits instead of having to open a separate PR. |
|
If you don't have write access to the repo, checking 'Allow edits by maintainers' on the PR accomplishes the same thing. |
Summary
OpenFHEParallelControls::SetNumThreads(n)(and any other runtimeomp_set_num_threads(n)) is silently ignored by the library's parallel hotregions: the process keeps using every core. This makes it impossible to cap
OpenFHE to fewer threads once the library is loaded.
The cause is a one-line issue in
GetThreadLimit, and the fix is one line.The bug
GetThreadLimit(n)returnsmin(n, machineThreads), wheremachineThreadsislatched once from
omp_get_max_threads()at static-init (library load) andnever refreshed. Every parallel hot region is emitted as:
#pragma omp parallel for num_threads(OpenFHEParallelControls.GetThreadLimit(size))Per the OpenMP spec, an explicit
num_threads(...)clause overrides theruntime
nthreadsICV. So even after a caller sets the limit at runtime — viathe library's own
OpenFHEParallelControls::SetNumThreads(), which callsomp_set_num_threads()—GetThreadLimitstill returns the stalemachineThreads, and the region runs on all cores anyway. There is noin-process way to cap the library.
The fix
Also clamp by the live
omp_get_max_threads():The added call is a cheap ICV read. When the runtime limit is left at the
machine default,
GetThreadLimitreturns exactly what it did before — nofunctional change to an uncapped build.
Reproduction (measured, not inferred)
Both builds below are stock upstream
main(ed361af); the only difference isthis one-line patch. Same machine (16 cores), both call
OpenFHEParallelControls.SetNumThreads(2), and the reprex counts the threadsactually spawned by OpenFHE's exact
num_threads(GetThreadLimit(size))clause:
GetThreadLimit(32768)The capped run is slower (4.87 s vs 2.22 s) precisely because it now uses 2
cores instead of 16 — the cap is real.
Zero-dependency reproduction of the mechanism
No OpenFHE needed — this replicates the exact
num_threads(GetThreadLimit(n))pattern and counts threads before/after the one-line change:
On a 16-core machine this prints
UPSTREAM -> 16,PATCHED -> 2.Full library-level reproduction
A self-contained program that drives the real library (calls
OpenFHEParallelControls.SetNumThreads(2), counts the threads OpenFHE's hotregion spawns, and runs a heavy CKKS
EvalMultworkload) is attached asthread_cap_repro.cpp. Build it against a stock install and a patched installand run both; the numbers above are its output.
Compatibility
machine default.
omp_get_max_threads()is a per-region ICV read. If its cost is aconcern for very fine-grained regions, it can be cached with an explicit
refresh hook — happy to adjust to your preference.