Repository navigation
Conversation
…333) `libfaer_v0_23_team_create` now builds one `threadteam::Pool` and the handle owns it. A call moves the pool into a team, runs inside it and moves it back out (`Team::with_par`), so the calling thread is outside the team between calls and a handle reused for N calls builds one pool rather than N. `set_threads` resizes the owned pool (a width of one releases it, widening rebuilds it) and `destroy` drops it. Before: 5 calls on a live handle cost 65 allocations (13 per call). After: 5, which is threadteam's own reservation. Owning a `threadteam::Team` in the handle instead would cost nothing per call, but threadteam 0.5 pins the thread that creates a team for the team's lifetime, so a C caller's thread would sit pinned between calls; the crate's affinity test observes that placement directly. Behavioural change: two threads may still call operations on one handle, but their calls take turns on the single pool instead of each building one. The lifetime rules (two threads, exclusive set_threads/destroy, use after destroy, fork) are written on `libfaer_v0_23_team_create` and in the regenerated `faer.h`; whether a handle inherited across `fork` should be detected is left undecided. The C API surface is unchanged, `NULL` is still serial, and a caller that never calls `create` still gets correct results.
…'s rules (#333) An operation called from inside another operation on the same thread — a C callback calling back into this API, which `SchurParams::recommended_shift_count` and its siblings do — would wait for the pool its own caller holds, and the mutex is not reentrant, so it never returned. A thread-local guard now gives that call `Par::Seq`: nesting is not parallel, it is correct. `then_some` cannot express the guard (it builds its value eagerly, so the untaken branch drops one and clears the flag its caller set), which the new test pins. The rest is what review of the diff found, checked against threadteam 0.5.1 and faer's own call sites: - the fork rule named a remedy that does not work: a forked child inherits threadteam's worker cache and its mutex, so a *fresh* handle can block too. `exec` before using a team in a child whose parent already ran one. - `destroy` returns the pool's workers to threadteam's process-wide cache rather than ending their threads, and resizing builds the new pool before the old one is dropped, so it cannot take the old one's workers back. - the allocation test asserted on the cheapest of eight calls instead of a mean of two batches: a member that parks makes the next reservation cost six allocations (the cold path), which is still not a pool. Measured per call on one handle after the first: [14019, 1, 1, 1, 1, 1, 1, 1], and 13 on every call before this change. - the shared-handle test now counts the calls inside the team and requires the maximum to be one, which is the serialisation the docs promise; the products alone would not show it.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: the full FFI suite and runtime EVD/fork reproducers; the allocation-probe worker timed out without a completed result. Findings target the supplied Reviewed head: |
…ed-call exception (#333) Review of the previous commit found the gate itself unsound: asserting on the cheapest of eight calls can fail correct code (a cold reservation on every call) and can pass a shape that rebuilds a pool on every other call. The test now measures two shapes over the same number of calls in one run — one handle for eight calls, eight handles of one call each — and compares their sums, so no single call's warm/cold state decides it. Tail 7 vs 158 here; with the implementation reverted to origin/main the same test reads 91 vs 112 and fails. The rest is doc accuracy the review also found: - the first call of a pool pays threadteam's first *acquisition*, not the pool's construction: `create` builds the pool, outside the measured region. - `set_threads` cannot hand the new pool the old pool's workers: the width changed, and threadteam's cache matches exact widths. - a call nested in another call takes no lock, so it is not ordered against another thread using the same handle. The two-thread rule says so now, and the test's own limit (it forces no overlap) is recorded where the claim is.
…eproducers (#333) Review r1 on 806e265, the three findings left open: - fork / worker termination: the pool used threadteam's shared workers, so `destroy` parked them in the process-wide cache instead of ending them, and a child forked after a `destroy` took that cached Rayon pool (without its threads) in its own `create` and waited forever in the affinity broadcast. The pool is now built with a `thread_name`, which makes its workers dedicated: the drop joins them and neither side touches the cache. `faer-ffi/tests/fork.rs` measures it: thread count after `destroy` 6 vs 2 and the destroy-fork-create child killed by SIGALRM before, all passing after. The `fork` rule in the docs now says what is measured: a child uses only handles it creates itself. - callback reentry: `an_evd_callback_may_call_back_on_the_same_handle` runs the review's case through `libfaer_v0_23_evd_f64` with a `recommended_shift_count` callback that multiplies on the same handle, at width 1 and in parallel, under a deadline. It fails (width 1, deadlock) with the `InCall` guard disabled and passes with it. - the allocation gate compared against eight handles of one call, which stopped discriminating once a `create` starts threads (the reverted implementation passed). The reference is now `faer::team::with_team` per call, what `origin/main` ran: tail 10 (42 with members parked) vs 91, reverted 98 vs 91. - rustfmt (1.96.0, committed rustfmt.toml): the lines this branch adds are formatted; no hunk of `--check` changes an added line (5 did on 0b83572). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes #333.
What changed
faer#332gave the C API a caller-owned handle,FaerV0_24_Team, but it carried only the width, soevery call ran
faer::team::with_team, which builds athreadteam::Pool, reserves its members anddrops the pool again at the end of that call.
The handle now owns one
threadteam::Pool:libfaer_v0_23_team_createbuilds it once (a width of one is sequential and owns nothing),threadteam::Team, runs inside it, and moves it back out beforereturning (
Team::with_par,faer-ffi/src/lib.rs:310),libfaer_v0_23_team_set_threadsis a setting on the owned object: it rebuilds the pool for the newwidth, and narrowing to one releases it (
faer-ffi/src/lib.rs:292),libfaer_v0_23_team_destroydrops it.The C API surface is unchanged: same three functions, same signatures, same entry points taking
const FaerV0_24_Team *.NULLis still serial, and a caller that never callscreatestill getscorrect results.
Why not own a
threadteam::TeamThat would make a call allocate nothing at all, but threadteam 0.5 pins the thread that creates a
team to
S[0]for the team's whole life (pin::TeamPin). A C caller's thread would then sit pinnedto one CPU between calls. The per-call
into_team/into_poolkeeps the caller outside the team, whichis the property
the_team_handle_is_entered_per_call_and_left_againalready observes through theaffinity mask.
What remains per call is threadteam's own reservation: one allocation (the id list) plus the affinity
syscalls of placing the leader for the call. Removing those needs a pool-lending API in threadteam,
which is an upstream change and out of scope here.
Lifetime rules
Written on
libfaer_v0_23_team_create(faer-ffi/src/lib.rs:2715), and therefore in the committed,regenerated
faer-ffi/faer.h:long as they write different elements. They do not run at once: the pool is a single object that
one call holds for its duration, so the second waits (
Team::with_par's mutex). A caller that wantsits own threads to be parallel wants a handle per thread. This is the behavioural change of the
issue; ffi: a caller-owned team handle instead of
Parby value (#98) #332 documented the handle as shareable for reading.set_threadsanddestroyare exclusive: no operation may be running on the handle, on anythread, while they are called.
destroythe handle is gone: any later operation,set_threadsor seconddestroyonthat pointer is a use after free.
destroy(NULL)is accepted and ignored.the same thread (the
SchurParamscallbacks, for one) gets no team and runs sequentially. Handing itone would make it wait for the pool its own caller holds, and the mutex is not reentrant, so it
would never return. Nesting is not parallel; it is correct.
forkwithoutexecis not supported. A forked child inherits threadteam's process-wide workercache and its mutex, so any handle there — the inherited one and a fresh one alike — can block on a
lock the parent held, or on workers it does not have. Create handles in the process that uses them,
and
execbefore using a team in a child whose parent already ran one.Undecided (deliberately left open, no code): whether
createshould record the creator's pid andrefuse to work in a process it did not create the handle in. That would turn the
forkcase above from"undefined, document
exec" into a reported failure, but it adds a user-visible behaviour (a status oran abort) to a boundary that currently has no error channel, so it wants an explicit decision rather
than a rider on this change.
Files
faer-ffi/src/lib.rs— the pool inTeam,Team::{pool_for,set_threads,with_par}, the rewrittenhandle docs, the allocation probe and its test.
faer-ffi/faer.h— regenerated (FAER_FFI=1 cargo +nightly build -p faer-ffi); the doc changesabove are what C callers read.
faer-ffi/Cargo.toml,Cargo.lock—faer-ffinamesthreadteamdirectly (four calls: build,into_team,exec,into_pool) instead of adding a pool-lending helper tofaer::team, whichwould be new public Rust API for one consumer and is the pattern
with_team's own docs recommend toa caller that owns a pool.
docs/worklogs/2026-11-09-ffi-team-pool.md— decisions, the rejected alternative, and what is notverified.
PATCHSET.md,docs/par-team-migration.md— the ledger row and the migration note for the handle.Verification
Hosted CI has not scheduled a single workflow run on this branch (
actions/runs?head_sha=...isempty for all three commits) while sibling PRs opened in the same minutes do have runs, so the
required checks are missing rather than failing. That is a repository/Actions condition, not something
this diff can fix; the crate's own gate above is what was run.
Not run: the
t4a-faerlib suite (nothing infaer/changed; hosted CI owns that matrix), any GPUwork (the CUDA jobs of this repository touch
faer, notfaer-ffi), Windows/macOS (threadteam doesnot pin there, so the placement test degrades as it already did), and
forkwithoutexec(documented, deliberately untested).
Review:
review: gpt-6.1-sol changes-requested (r1) -> changes-requested (r2) -> fixed in 0b835728Two independent rounds on
openai/gpt-6.1-sol, read-only, over the diff againstorigin/main.Round 1 (
806e265e) — 2 IMPORTANT, 4 MINOR, all verified against the source and all fixed:a C callback nesting a call on the same handle deadlocked on the non-reentrant mutex; the
forkrulenamed a remedy (a fresh handle) that does not work because threadteam's worker cache and its mutex are
inherited; and three places where docs or tests claimed more than they showed (a two-batch allocation
bound, a
first > 0assertion that measured nothing, a "serialisation is tested" claim the productsdid not show).
Round 2 (
0808c92d) — 1 IMPORTANT, 3 MINOR, all verified and all fixed:the allocation gate could fail correct code (a cold reservation on every call) and could pass a shape
that rebuilt a pool every other call, so it became a paired same-run comparison of two shapes; a call
nested in a callback takes no lock and so is not ordered against another thread using the same handle,
which the two-thread rule now says; and two doc claims (the first call "costs the pool's
construction", a resize "takes the old workers") were wrong and are gone.
Nothing was dismissed. Both rounds' reading conclusions agree that the ownership is sound on the
normal, panic and unwind paths, that
InCalldoes not destroy its guard eagerly, and thatforkneeds
execrather than a pid check.