Skip to content

fix for issue1232 - #1233

Merged
dsuponitskiy merged 1 commit into
devfrom
issue1232
Aug 5, 2026
Merged

fix for issue1232#1233
dsuponitskiy merged 1 commit into
devfrom
issue1232

Conversation

@pascoec

@pascoec pascoec commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1232.

Problem

OpenFHEParallelControls::SetNumThreads(n) is silently ignored by the library's
parallel hot regions. Every hot region is emitted as
#pragma omp parallel for num_threads(GetThreadLimit(size)), and per the OpenMP
spec an explicit num_threads(...) clause overrides the nthreads ICV that
omp_set_num_threads() sets. Since GetThreadLimit() compares only against a
thread count latched once at library load, there is no in-process way to cap the
library. Related defects in parallel.h:

  • Thread limits are per-thread, not process-global — the nthreads ICV is
    thread-local, so a cap set on one thread never applies to regions entered from
    another (e.g. FHE work on a worker pool). This is also why clamping by a live
    omp_get_max_threads() inside GetThreadLimit() would be insufficient.
  • UnitTestStart() halves the wrong base — it reads the current mutable ICV, so
    repeated calls compound (16 → 8 → 4) and any prior cap skews it; its throttle
    is itself bypassed by the GetThreadLimit regions.
  • UnitTestStop() unconditionally restores machineThreads, stomping any cap
    the application had set.
  • omp_set_num_threads(0) is reachable (via SetNumThreads(0), or
    UnitTestStart() with one thread available); the spec requires a positive
    argument.
  • Several parallel regions (matrix, matrixstrassen, dgsampling, CKKS scheme
    switching) have no num_threads clause at all, so they follow only the
    thread-local ICV.

Changes

src/core/include/utils/parallel.h:

  • New std::atomic<int> threadLimit holds the process-global cap;
    GetThreadLimit() mins against it (relaxed load, ~0.3 ns — measured). A cached
    member is preferred over reading the live ICV because the ICV is per-thread:
    after omp_set_num_threads(2) on one thread, another thread still reads the
    machine default.
  • SetNumThreads() clamps to [1, machineThreads], stores the cap, and still
    sets the ICV for clause-less regions.
  • Enable()/Disable() route through SetNumThreads() so they actually govern
    the hot regions.
  • UnitTestStart() uses GetNumProcs()/2 (stable hardware base, immune to
    OMP_NUM_THREADS over/undersubscription) and saves the current cap;
    UnitTestStop() restores it.
  • The constructor no longer calls Enable(); constructing a ParallelControls
    has no side effects on global OpenMP state.
  • GetNumThreads() is now a relaxed atomic load.

Added num_threads(OpenFHEParallelControls.GetThreadLimit(<trip count>)) to the
27 clause-less regions in matrix.h, matrix-impl.h, matrixstrassen.h,
matrixstrassen-impl.h, dgsampling-impl.h, and
ckksrns-schemeswitching.cpp, so all regions obey the same process-global cap.
The Strassen NUM_THREADS latch now reads
OpenFHEParallelControls.GetNumThreads() so its schedule chunk math matches
the capped team size.

@pascoec pascoec added this to the Release 1.6.0 milestone Aug 3, 2026
@pascoec pascoec self-assigned this Aug 3, 2026
@pascoec pascoec added the bug Something isn't working label Aug 3, 2026
@pascoec pascoec linked an issue Aug 3, 2026 that may be closed by this pull request
@pascoec
pascoec requested review from bnaras and dsuponitskiy August 3, 2026 19:00

@bnaras bnaras left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pascoec!

I built the issue1232 branch (46732b2) on macOS arm64 (16 cores, Homebrew libomp) and ran it against the harness from #1224, extended with your worker-thread scenario.

Compared three builds: stock dev, the #1224 live-ICV clamp, and this PR. The probe spawns the exact hot-region clause — num_threads(OpenFHEParallelControls.GetThreadLimit(1 << 15)) — and counts the team size; the corroborating workload is 50 top-level CKKS EvalMults (depth 20, ring dim 2^16) under SetNumThreads(2).

scenario stock dev #1224 clamp this PR
bare omp_set_num_threads(2), region on main thread 16 2 (same-thread only) 16 — as your usage note says
SetNumThreads(2), region on main thread 16 2 2
SetNumThreads(2) on main, region entered from a std::thread worker 16 16 2
EvalMult workload CPU/wall under SetNumThreads(2) 4.26 1.89 1.89

You're right about the thread-local ICV — reproduced against my own patch: after capping on the main thread, omp_get_max_threads() from a fresh std::thread reports 16 and the hot region spawns a full 16-thread team. The process-global atomic in this PR closes exactly that hole: the same worker-thread entry spawns 2.

Also read through the rest of the diff: every library-side parallel region now carries the GetThreadLimit clause (the only remaining bare #pragma omp parallel sites are in unittests, examples/parallel.cpp, and commented-out code), and the Strassen NUM_THREADS latch now agrees with the capped team size its schedule chunk math assumes.

Confirmed: this resolves my use case. The R package needs an in-process cap (CRAN's check farm limits packages to 2 cores, and a package cannot set the environment before its own process starts), and SetNumThreads(2) under this PR caps the hot path process-wide, with the workload ratio matching what the #1224 clamp achieved on the main thread. I'll switch the R wrapper from bare omp_set_num_threads() to OpenFHEParallelControls::SetNumThreads() and drop our carried patch when we next rebase onto a release containing this.

Noted on workflow as well — future PRs will come from an in-repo branch (or with "Allow edits by maintainers" checked).

@dsuponitskiy
dsuponitskiy merged commit 4be14b0 into dev Aug 5, 2026
19 checks passed
@dsuponitskiy
dsuponitskiy deleted the issue1232 branch August 5, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential issues with parallel.h

3 participants