178 lines
10 KiB
Markdown
178 lines
10 KiB
Markdown
# Shared pool vs raw `std::thread` — why the raytracer deadlocks and where `2N+8` comes from
|
||
|
||
> Companion to [`README.md`](README.md) (the camera-move deadlock fix). That doc describes *what*
|
||
> was fixed; this one explains the underlying architecture: upstream KiCad's **two** threading
|
||
> patterns, why only one of them is WASM-hostile, the Worker ledger behind the
|
||
> `PTHREAD_POOL_SIZE = hardwareConcurrency*2+8` constant, why the June-30 threading revert did
|
||
> **not** already cover this, and the upstream-able follow-up that would make the whole deadlock
|
||
> class impossible by construction.
|
||
>
|
||
> Line numbers are against the branch at fix time (KiCad 10.0.4; the cited KiCad files are
|
||
> byte-identical to upstream).
|
||
|
||
## 1. Web Workers are the scarce resource — and a missing one can't be waited for
|
||
|
||
Every pthread in the browser runs inside a Web Worker. Emscripten obtains one of two ways:
|
||
|
||
- **Pre-warmed** (`-sPTHREAD_POOL_SIZE`): Workers created and fully loaded at startup, parked
|
||
idle. `pthread_create` against one is a single `postMessage` that the Worker's **own** event
|
||
loop receives — the main thread does not participate.
|
||
- **On-demand** (the fallback, since `-sPTHREAD_POOL_SIZE_STRICT=0`): `new Worker()` at
|
||
`pthread_create` time. The Worker must fetch + instantiate the (~190 MB) module, then post
|
||
*"loaded"* back to the **main thread**, whose handler posts *"run"*. The thread only starts
|
||
once the main thread returns to its event loop.
|
||
|
||
That asymmetry is the whole story. A Worker shortfall is not a slowdown: if the spawner then
|
||
blocks the main thread waiting for the spawned threads (the raytracer's busy-wait join), the
|
||
on-demand boot handshake can never complete — circular wait, tab frozen. See
|
||
[`README.md` §3](README.md) for the deadlock diagram.
|
||
|
||
## 2. Upstream KiCad has two threading patterns, side by side
|
||
|
||
**Pattern 1 — the shared pool.** `KICAD_SINGLETON::Init()` (`common/singleton.cpp:60-62`)
|
||
creates one `BS::priority_thread_pool` (`include/thread_pool.h:31`) with
|
||
`hardware_concurrency()` threads (`bs_thread_pool.hpp:1949-1954`) **once, at startup**. The
|
||
threads park on a condition variable; subsystems *submit tasks*:
|
||
zone fill (`pcbnew/zone_filler.cpp:736,825`), DRC
|
||
(`drc/drc_test_provider_copper_clearance.cpp:697`), connectivity (`pcbnew/board.cpp:1159`),
|
||
footprint loading (`pcbnew/footprint_info_impl.cpp:218`) — and, notably, **the raytracer's own
|
||
main tracing pass**, `renderTracing`
|
||
(`3d-viewer/3d_rendering/raytracing/render_3d_raytrace_base.cpp:250-288`):
|
||
|
||
```cpp
|
||
thread_pool& tp = GetKiCadThreadPool();
|
||
// ...
|
||
for( size_t i = 0; i < tp.get_thread_count(); ++i )
|
||
futures.push_back( tp.submit_task( processBlocks ) );
|
||
futures.wait();
|
||
```
|
||
|
||
Running a task = queue-push + condvar-notify to threads whose Workers **already exist**. No
|
||
`pthread_create` ever happens after startup.
|
||
|
||
**Pattern 2 — raw `std::thread`.** Older 3D-viewer code predating KiCad's pool adoption creates
|
||
`std::max( hardware_concurrency(), 2 )` fresh threads **per pass**, detaches them, and busy-waits
|
||
(`while( threadsFinished < N ) sleep_for( 10ms )`) on the calling (= browser main) thread:
|
||
|
||
| Site | Spawn / join | Runs when |
|
||
|---|---|---|
|
||
| `renderPreview` | `render_3d_raytrace_base.cpp:818` / `:1401` | **every camera move** (the interaction pass) |
|
||
| `postProcessShading` | `render_3d_raytrace_base.cpp:707` / `:727` | end of a full trace |
|
||
| `postProcessBlurFinish` | `render_3d_raytrace_base.cpp:756` / `:789` | end of a full trace |
|
||
| `BOARD_ADAPTER::createLayers` helpers | `create_layer_items.cpp:1241,1742` | scene build (viewer open / board change) |
|
||
| `IMAGE::EfxFilter` | `image.cpp:488` | not on the raytrace hot path |
|
||
|
||
The WASM-critical difference between the patterns is **when `pthread_create` runs**: Pattern 1
|
||
calls it at startup, when the main thread is idle and the pre-warmed bag is full. Pattern 2 calls
|
||
it at **render time** — mid-interaction, with the main thread about to block in the join. Only
|
||
Pattern 2 can ever need an on-demand Worker boot, and it needs it at the worst possible moment.
|
||
|
||
It is a wry detail that upstream already ported the *slow* pass (`renderTracing`, the full-quality
|
||
progressive trace) to the pool, while the pass that runs on **every camera move**
|
||
(`renderPreview`) is still raw — which is exactly why the deadlock bites on interaction.
|
||
|
||
## 3. The Worker ledger — deriving `2N+8`
|
||
|
||
The invariant the build must satisfy: **pre-warmed Workers ≥ peak simultaneous live pthreads.**
|
||
Any shortfall silently switches the excess threads to on-demand boot (`STRICT=0`), i.e. to the
|
||
deadlock path. On an 8-core machine (`N = 8`):
|
||
|
||
| Consumer | Workers | Held |
|
||
|---|---|---|
|
||
| KiCad shared pool (Pattern 1, `singleton.cpp:60`) | `N` = 8 | forever (threads never exit) |
|
||
| One raw-thread pass in flight (Pattern 2) | `N` = 8 | for the pass duration |
|
||
| Margin (`+8`) | 8 | see below |
|
||
| **Pre-warmed total** | **`2N+8` = 24** | |
|
||
|
||
The `2N` is *derived*: two independent populations, both sized off `hardware_concurrency` by
|
||
upstream, provably alive at the same time (the pool threads never exit; a raw pass spawns its full
|
||
set before joining). With the old `PTHREAD_POOL_SIZE = N`, the free warm count at render time was
|
||
**exactly zero** — every `renderPreview` thread went on-demand, which is why the freeze was 100%
|
||
reproducible, not flaky.
|
||
|
||
The `+8` is *margin*, not derived. It absorbs:
|
||
|
||
- **Recycle lag** — an exited pthread's Worker returns to the warm bag only after the **main
|
||
thread** processes its exit message. Back-to-back passes (preview → trace slices → post-process)
|
||
can spawn while a few Workers from the previous pass are still in limbo.
|
||
- **Stray long-lived threads** outside the pool that each permanently hold a Worker (e.g. the
|
||
font-list poller, `common/widgets/font_choice.cpp:100`, if active in a given app).
|
||
- Small-core machines, where `2N` alone is a small absolute number.
|
||
|
||
Honest assessment: this is a **capacity answer to a structural problem**. The constant chases the
|
||
runtime behavior of upstream code we deliberately don't patch — if a future KiCad adds another
|
||
raw-thread site or resizes its pool, `2N+8` silently goes stale. The guard is the regression test
|
||
(`tests/kicad/3d-viewer-deadlock.spec.ts`), not construction. §5 is the structural fix.
|
||
|
||
## 4. "Didn't the June-30 threading revert already fix this?"
|
||
|
||
No — it's what *exposed* this, and the distinction matters:
|
||
|
||
1. **Route C** (kicad `4ccabfd5c3`, 2026-06-18): multi-threaded wasm didn't survive Asyncify, so
|
||
the raytracer passes were forced single-threaded behind `#ifdef __EMSCRIPTEN__` and the shared
|
||
pool got an inline-`detach_task` shim. No parallelism → no Worker demand → no deadlock.
|
||
2. **Native EH** (root `c1ef489`/`ee01642`, 2026-06-29) removed the Asyncify-rewind crash that
|
||
motivated those hacks.
|
||
3. **The threading revert** (kicad `4f42d0b328` + root `71807db`, 2026-06-30) restored the four
|
||
files **byte-identical to upstream** and linked `wasm/shims/nanosleep_yield.c` as the guard:
|
||
the busy-wait join yields via Asyncify so on-demand Workers *can* boot.
|
||
|
||
The revert restored upstream's **mixed** state — Pattern 1 for `renderTracing`, Pattern 2 for
|
||
everything in the §2 table. It did not (and could not) route the raw passes through the pool,
|
||
because **upstream itself never has**. "Original upstream threading" *is* the raw-thread preview
|
||
pass. And the revert's guard, the nanosleep yield, turned out to hold only in some call chains:
|
||
it boots Workers fine at viewer-open (scene build + first render, reached via the yielding pump),
|
||
but not reliably in interaction paint chains — hard freeze, or `Aborted(invalid state)` when the
|
||
unwind is illegal ([`README.md` §3–4](README.md)). The June-30 e2e suite was green (63/63) because
|
||
no test then dragged **on the GL canvas** — the only drag test moved the DOM titlebar; the
|
||
camera-move path had no coverage until `3d-viewer-deadlock.spec.ts`.
|
||
|
||
So the layering on this branch is: the revert made upstream threading *run*; the `2N+8` pre-warm
|
||
makes the fragile on-demand path *unnecessary*; the shim and the mouse-button `Paint()` defer
|
||
remain as fallback layers beneath it.
|
||
|
||
## 5. Follow-up — finish the port upstream already started
|
||
|
||
The structural fix is to convert the §2 table's raw-thread sites to the pattern `renderTracing`
|
||
already demonstrates ~400 lines above them in the same file: submit the same block-consuming
|
||
lambdas to `GetKiCadThreadPool()` instead of spawning threads.
|
||
|
||
```cpp
|
||
// today (renderPreview and friends): spawn + detach + busy-wait
|
||
for( size_t ii = 0; ii < parallelThreadCount; ++ii )
|
||
std::thread( [&]() { /* consume blocks via nextBlock.fetch_add */ } ).detach();
|
||
while( threadsFinished < parallelThreadCount )
|
||
std::this_thread::sleep_for( std::chrono::milliseconds( 10 ) );
|
||
|
||
// ported: the renderTracing pattern (render_3d_raytrace_base.cpp:284-288)
|
||
thread_pool& tp = GetKiCadThreadPool();
|
||
BS::multi_future<void> futures;
|
||
for( size_t i = 0; i < tp.get_thread_count(); ++i )
|
||
futures.push_back( tp.submit_task( /* same lambda */ ) );
|
||
futures.wait();
|
||
```
|
||
|
||
Consequences:
|
||
|
||
- **The deadlock class disappears by construction** — no `pthread_create` after startup, so no
|
||
on-demand Worker boot can ever be needed, regardless of pool sizing, call chain, or Asyncify
|
||
state. Not "unlikely"; impossible.
|
||
- `PTHREAD_POOL_SIZE` drops back to plain `navigator.hardwareConcurrency` — no multiplier, no
|
||
margin, no staleness risk. The nanosleep shim and the `Paint()` defer become pure
|
||
belt-and-braces.
|
||
- Native desktop benefits too: no create/destroy churn of ~N OS threads per pass per progressive
|
||
frame, and the raytracer becomes consistent with every other KiCad subsystem.
|
||
|
||
Wrinkles, all manageable: the main-thread wait can stay `futures.wait()` (proven viable in this
|
||
very build — `renderTracing` does it today) or keep the counter + `sleep_for` loop (which the
|
||
shim turns into a UI-pumping yield); every §2 call site runs on the main thread, so there is no
|
||
nested-submission (pool-task-waiting-on-pool-task) hazard; it's a priority pool if preview passes
|
||
ever need to jump the queue.
|
||
|
||
**Why it's deferred, not done here:** those exact files were made byte-identical to upstream the
|
||
day before this fix (kicad `4f42d0b328`) — re-diverging them reverses that cleanup and walks the
|
||
fork away from upstream again (the standing fork policy; `scripts/kicad-diff-stats.sh` polices
|
||
it). The right vehicle is an **upstream KiCad merge request** — "port the 3D raytracer's remaining
|
||
raw-`std::thread` passes to the shared thread pool, like `renderTracing`" is an upstream-quality
|
||
cleanup with native benefits. If it lands, the fork inherits it on the next rebase and the pool
|
||
expression collapses back to `N`.
|