Use coot::get_max_number_of_threads() in calc_atom_map_edcalc - #384
Open
martinemnoble1 wants to merge 1 commit into
Open
Use coot::get_max_number_of_threads() in calc_atom_map_edcalc#384martinemnoble1 wants to merge 1 commit into
martinemnoble1 wants to merge 1 commit into
Conversation
calc_atom_map_edcalc() sized its thread count from std::thread::hardware_concurrency() directly, so set_max_number_of_threads() and COOT_N_THREADS had no effect on it. get_max_number_of_threads() still falls back to the system CPU count when nothing has been set, so callers that never set a limit are unaffected; callers that do set one are now honoured. On a workstation an over-large thread count is only a scheduling inefficiency. Under WebAssembly it is a correctness problem: pthreads are drawn from a fixed-size pool established at link time (-sPTHREAD_POOL_SIZE), and when it is exhausted emscripten's fallback needs the JS event loop to run before the thread can start -- which this function's spawn-then-join loop never allows. The result is a deadlock rather than a slowdown, and because the count tracked core count it was hardware-dependent. Measured in an embedded Moorhen session, reached via density_correlation_analysis -> map_to_model_correlation_per_residue -> calc_atom_map. The thread-count setting itself was working: instrumenting both accessors to print the value and the address of coot_n_threads showed set(3) and get -> 3 on the same object, while coot's own timing line in the same run reported "n_threads 8". After this change it reports "n_threads 3", peak concurrent pthreads over a full session drops from 13 to 8, and a build with PTHREAD_POOL_SIZE=8 goes from deadlocking on map load to loading cleanly. The same pattern appears in seven other places that are not part of the WebAssembly build and are left unchanged here: coot-utils/crowther.cc (three), ligand/molecular-replacement.cc (two), docking/semiflex-refine.cc and docking/rigid-body-dock.cc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
calc_atom_map_edcalc()sizes its thread pool fromstd::thread::hardware_concurrency()directly:
so
set_max_number_of_threads()(andCOOT_N_THREADS) have no effect on it. Thispatch changes it to
coot::get_max_number_of_threads(), which still falls back tothe system CPU count when nothing has been set — so callers that never set a limit
behave exactly as before, and callers that do set one are now honoured.
Why this matters
On a workstation an over-large thread count is a scheduling inefficiency. Under
WebAssembly it is a correctness problem: pthreads come from a fixed-size pool
established at link time (
-sPTHREAD_POOL_SIZE). When that pool is exhausted,emscripten's fallback requires returning to the JS event loop before the thread can
start — and
calc_atom_map_edcalc's spawn-then-join()loop never does. The resultis a deadlock rather than a slowdown.
Because the count tracked core count, the failure was hardware-dependent: it
reproduced on a 10-core machine and would have appeared on any machine with more
cores regardless of how the pool was sized.
Evidence
Reached via
density_correlation_analysis→map_to_model_correlation_per_residue→
calc_atom_map, in an embedded Moorhen session.The thread-count setting itself was working correctly. Instrumenting both accessors
to print the value and the address of
coot_n_threads:Same object, correct value. But coot's own timing line in the same run reported:
After the change, on the same structure:
Tracing emscripten's worker pool over a full session (model + map + validation):
thread_pool(8))PTHREAD_POOL_SIZE=8Related sites — not changed here
std::thread::hardware_concurrency()is used the same way in seven other places.None are compiled into the WebAssembly build, so I have no evidence from them and
have deliberately left them alone — but they will equally ignore
set_max_number_of_threads()for desktop and Python callers, so you may want themswept in the same change:
coot-utils/crowther.cc:473, 587, 681ligand/molecular-replacement.cc:380, 543docking/semiflex-refine.cc:987docking/rigid-body-dock.cc:615Two nearby thread counts are hardcoded rather than hardware-derived
(
coot-utils/coot-map-utils.cc:4184= 4,:4782= 8). They also bypass thesetting, but being fixed they cannot scale with core count, so they are a milder
case.
Also noticed
map_to_model_correlation_stats_per_residue_runlogs an error when the thread countexceeds the number of work ranges — with 3 threads and 2 ranges it emits:
The loop runs to
n_threadsbut only pushes work whilei_thread < ranges.size(),so the final iteration logs an error on a case that is expected. Harmless, but noisy.
Happy to fix separately if useful.
Testing
Built for WebAssembly (emscripten 6.0.0, 32- and 64-bit) and exercised through
Moorhen: model load, map load, contouring, and the density-correlation validation
path. No behaviour change observed other than the thread count; validation output
is unchanged.
I have not been able to test the desktop or Python builds — worth a second pair of
eyes on whether any caller depends on this function saturating the machine by
default. Note the fallback preserves that behaviour unless a limit is explicitly set.