Skip to content

fbench: a per-cell time limit, and the check's time recorded apart (#57) - #58

Open
shinaoka wants to merge 3 commits into
mainfrom
fix/57-cell-timeout
Open

shinaoka wants to merge 3 commits into
mainfrom
fix/57-cell-timeout

Conversation

@shinaoka

@shinaoka shinaoka commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Closes part of #57: the per-cell limit (item 1) and the check's time in the record (item 3). Item 2 — whether the orthogonality check samples instead of running the full O(n^3) product — is the maintainer's call and is not in this PR. The verification method is unchanged.

What happened, and what this fixes

The weekend CPU campaign ran 27.6 h on one n = 4096 cell: fbench::shared::gram_residual, the serial triple loop inside svd_thin's check. Two things made that worse than slow:

  • no per-cell limit anywhere on the path pinned.sh invokes, so nothing cut it off and master.sh waited for a DONE that never came;
  • fbench writes its CSV once, at the end of main, so the cells that had already finished left no row either. "Not measured" and "hung" were the same thing in the run directory.

1. --cell-timeout SECS

  • Default 3600 s. --cell-timeout wins over FBENCH_CELL_TIMEOUT, which wins over the default; 0 disables it. The campaign needs no recorder change, and a host or pilot can lower it without editing the invocation the recorder composes.
  • The clock covers one cell — one Op, one CSV row: armed before the cell, disarmed after it, so a cut ends that cell only and the loop continues. The rows after a cut are still measured (master.sh is not the only thing that used to stop).
  • Polled where the harness's own loops run: the timing loop (prime and each repetition) and the serial reference math a check executes — shared::gram_residual, gram_adj_residual, mul, mul_adj_b. gram_residual's outer loop is exactly where the 27.6 h went.
  • The clock is thread_local! (a cell runs on one thread from its first prepare to its check) — no atomics, no watchdog thread, no signal, nothing to clean up on the measured cores.
  • A cut cell is a row: status=timeout, note timeout 3600. Not failed, because a cell lost to the limit and a cell caught giving a wrong answer are not the same event. Every existing reader treats anything other than ok/skipped as "no number", and timing_evidence refuses to publish a run that carries one, so a timeout cannot be published as a measurement. In verify it prints TIMEOUT <cell> instead of MISMATCH <cell>.

Limit of the mechanism, stated rather than hidden: a single library call already in progress cannot be interrupted. The cell is cut at the next poll point, and every loop that can grow without bound has one. A cell that blocks inside one call for an hour is cut an hour late, not never.

2. timed_ms and check_ms

Two appended columns, in both run and verify (appended after the profile column that #56 added, so nothing that already read the header moves): the timed calls summed, and the check that runs once afterwards outside the clock. Appended so the columns benchmarks/campaign.yaml declares and the profile column (which report_run.read_rows requires and timing_evidence reads) are untouched; python3 scripts/summarize_samples.py reads the new files unchanged.

Measured, n = 512, 1T, the four svd_thin cells: timed_ms 110, 99, 99, 99 against check_ms 1651, 1653, 968, 966. The check was 15x the measurement and no row said so.

Red -> green, measured

Red, before this commit (release build, fbench run --threads 1 --n 512,1024 --family svd_thin --dtype f64 --rows faer-fork --reps 1 --prime-ms 0), under an outer timeout 60:

rc=124  wall=60.01s
wc: /tmp/red2.csv: No such file or directory

The four n = 512 cells finish in ~5.8 s; the n = 1024 check runs on; the process is killed; no CSV at all.

Green, after, the same invocation with --cell-timeout 20:

regime,family,dtype,order,m,n,k,batch,row,threads,total_ms,per_item_us,status,note,samples_ms,rayon_threads,vendor_threads,profile,timed_ms,check_ms
linalg-small,svd_thin,f64,c,512,512,,1,faer-fork,1,109.609022,109609.022000,ok,,[109.609022],1,,,109.609022,1651.189532
linalg-small,svd_thin,f64,c,512,512,,1,faer-fork,1,98.734609,98734.609000,ok,in_place,[98.734609],1,,,98.734609,1652.513178
linalg-small,svd_thin,f64,f,512,512,,1,faer-fork,1,99.499301,99499.301000,ok,,[99.499301],1,,,99.499301,968.097202
linalg-small,svd_thin,f64,f,512,512,,1,faer-fork,1,98.628246,98628.246000,ok,in_place,[98.628246],1,,,98.628246,965.802108
linalg-small,svd_thin,f64,c,1024,1024,,1,faer-fork,1,,,timeout,timeout 20,,1,,,,
linalg-small,svd_thin,f64,c,1024,1024,,1,faer-fork,1,,,timeout,timeout 20,,1,,,,
linalg-small,svd_thin,f64,f,1024,1024,,1,faer-fork,1,647.371428,647371.428000,ok,,[647.371428],1,,,647.371428,7705.960268
linalg-small,svd_thin,f64,f,1024,1024,,1,faer-fork,1,637.360669,637360.669000,ok,in_place,[637.360669],1,,,637.360669,7702.259481

rc 1 (a cell failed, nothing is publishable). The cut cells are rows, and the F-order n = 1024 cells after them are measured — that is the "cut one cell, keep going" requirement, end to end on the real pathological path at a size that costs a minute instead of a weekend.

fbench verify on the same grid writes the same two columns in its census CSV and prints TIMEOUT for the two cut cells, all comparisons within tolerance absent, rc 1 — which is what makes the recorder stop rather than publish.

Checks run

  • cargo test --release -p fbench --features fork-par-shape,fbench/fork-team (patched to the local extern/ checkouts): 10 passed, 0 failed — including two new tests: a fake cell whose check sleeps past the limit comes back timeout with the following cell ok, and the check's time is reported apart from the timed calls.
  • cargo clippy on the same configuration: no new warnings.
  • rustfmt --check on the three touched files only (the rest of the tree is not rustfmt-clean and is left alone).
  • The repo gate: python3 -m unittest discover -s tests -v (5 passed), python3 -m campaign_toolkit.declarations (ok).
  • Rebased onto 01a1cb8: the two new columns now follow the profile column fix(record_run): refuse a host the declared profile does not describe (#55) #56 added; all of the above is re-run at 8c659f1.
  • Reader compatibility: scripts/summarize_samples.py on the new CSV (reports the timeout row as a failed measurement, as it always did for any non-ok status) and report_run.read_rows' declared-column check against benchmarks/campaign.yaml (no missing column; the new two are ignored extras).

Design

docs/reviews/20261109-cell-timeout.md, and the harness contract in docs/results.md now names the flag, the status and the two columns.

Not in this PR: making the orthogonality check proportional (#57 item 2), and writing the CSV incrementally so a killed process still leaves the rows it finished — both are separate decisions.

The harness builds from the committed lock, and CI now says so (#59)

cargo build --locked did not work at this branch, and nothing built fbench in CI, so it had gone unnoticed. Two separate faults:

  • The lock was a path-patched resolution. t4a-faer and t4a-faer-traits carried no source line — it was written by a measured build that patched them to local checkouts — so --locked refused it before compiling: error: cannot update the lock file ... because --locked was passed.
  • Then the wrong fork generation was compiled. Once re-resolved, the open-ended >=0.24, <1.0 picks 0.33.0, and the default features still selected the pre-0.25 generation (Par is Copy, Par::rayon, workspace queries take a Par). That, not the API, is the 21 errors: with fork-par-shape + fork-team the same backend.rs compiles unchanged against 0.33.0.

So: the lock is re-resolved from the registry at the pinned fork's version (0.28.0), the two generation features are the default (the registry arm has no checkout to read a generation from), and a measured build keeps reading the generation from its checkout — scripts/build_for_faer_rev.sh now passes --no-default-features plus its own detection, which leaves BUILD_FEATURES byte-identical to before (fbench/link-mkl,fbench/fork-par-shape,fbench/fork-team for the pinned checkout).

CI gains one job, harness, running cargo build --locked (57 s, green here). It is not a measurement gate: it builds the one configuration nobody records. --locked is the point — the lock is what a reader gets, so a lock the harness cannot compile against fails there and not nowhere.

Red, measured locally with the lane's own command, before this commit:

$ cargo build --locked                     # committed lock
error: cannot update the lock file ... because --locked was passed
$ cargo build --locked                     # after re-resolving to 0.33.0
error: could not compile `fbench` (bin "fbench") due to 21 previous errors

Green, after: cargo build --locked → 0 errors; cargo test --locked → 13 passed, 0 failed; the campaign build for the pinned revisions 325e482/0539947f (MKL arm) → exit 0.

Not measured: the hosted lane on a rotten lock (only the local runs above were made with that state), and any number — nothing here was timed.

review: gpt-6.1-sol — 1 IMPORTANT, verified pre-existing and left unfixed (fbench/src/backend.rs:45,49: the pre-0.25 aliases spell Par<'static>, which 0.24 declares without a lifetime; that arm is not reachable from any lock, since resolving a pre-0.26 crate fails on faer_fork/threadteam — report it as its own change), plus its one delete-list item applied (AGENTS.md no longer says the harness is unwritten).

One weekend campaign ran 27.6 h on a single `n = 4096` cell -- the serial
`gram_residual` inside `svd_thin`'s check -- and finished with nothing to show
for it: there was no per-cell limit, so nothing cut it off, and the run's CSV is
written once at the end, so even the cells that had finished left no row. A cell
that cannot finish and a cell that was never measured looked exactly alike.

`run` and `verify` now take `--cell-timeout SECS` (`FBENCH_CELL_TIMEOUT`, default
3600, `0` disables). The clock covers one cell -- one `Op`, one CSV row -- is
armed before it and disarmed after it, and is polled by the loops the harness
owns: the timing loop, and the serial reference math a check runs
(`shared::gram_residual`, `gram_adj_residual`, `mul`, `mul_adj_b`), which is where
this cell spent its time. A cut cell is recorded as `status=timeout` with the
limit in its note and the run continues, so it is a row and not a hole, and it is
not confused with a wrong answer. A library call already in progress cannot be
interrupted; the cell is cut at the next poll point.

The check's cost is now in the record: `run` and `verify` write two appended
columns, `timed_ms` (the timed calls, summed) and `check_ms` (the check that runs
once after them, outside the clock). Appended, so the declared columns readers
require are untouched. Measured at `n = 512` on the four `svd_thin` cells: 108,
98, 99 and 98 ms of timed region against 1667, 1672, 969 and 971 ms of check --
the check was 15x the measurement and no row said so.

Red, before (release build, `--n 512,1024 --threads 1 --reps 1`): the four
`n = 512` cells finish in ~5.8 s, the `n = 1024` check runs on, and an outer
`timeout 60` kills the process (rc 124) with no CSV at all. Green, after, the
same invocation with `--cell-timeout 20`: eight rows, the four `n = 512` cells
`ok` with both times, the two `n = 1024` C-order cells `timeout 20`, and the two
F-order cells after them still measured.
@shinaoka
shinaoka force-pushed the fix/57-cell-timeout branch from 62b7e18 to 8c659f1 Compare November 9, 2026 08:58
@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 4
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • [P1] Multithreaded upstream cells execute without the armed deadline — fbench/src/main.rs:876-879, fbench/src/main.rs:175-178, fbench/src/timing.rs:14-21. run_cell arms thread-local state on the calling thread, but spindle::with_lock enters a Rayon scope and executes timing::measure on a worker whose deadline is None. Reproduction: arm a 10 ms deadline, enter spindle::with_lock(2, ...), sleep 20 ms, and call expired() inside and outside the closure; observed results were false on the worker and true on the caller. An isolated harness using the unchanged PR definitions also returned status=ok, check_ms=1225.945916 from run_cell(..., true, 2, "faer-upstream", 0, 1, 1), despite its check explicitly polling after sleeping past one second. Arm, inspect, and disarm the clock inside the execution context that actually runs the cell.

  • [P1] The last repetition and a successful check can exceed the limit and still produce an ok row — fbench/src/timing.rs:94-111, fbench/src/main.rs:879-886, fbench/src/main.rs:898-909. The timing loop polls only before repetitions; neither its success path nor verification’s post-check success path checks the deadline again. Reproduction: create an Op whose run sleeps 1100 ms and returns Ok(()), with no-op prepare/check, then call run_cell(..., true, 1, "faer-fork", 0, 1, 1); the isolated harness returned ok with timed_ms=1122.711375. Moving the sleep into a successful check and using verification mode also returned ok, with check_ms=1178.122667. This is not merely delayed interruption: after the call returns, the expired cell is accepted as a measurement. Poll after each completed stage and before accepting success.

  • [P2] Verification’s timed_ms includes preparation, unlike run mode and the documented “single call” metric — fbench/src/main.rs:889-892, docs/reviews/20261109-cell-timeout.md:47-48. The verification stopwatch starts before ops.prepare(), whereas run mode starts it afterward. Reproduction: use an Op with a 30 ms sleep in prepare and no-op run/check, then invoke verification through run_cell; the isolated harness recorded timed_ms=39.181208 for the no-op run. Start the verification stopwatch after successful preparation so both CSV modes describe the same region.

  • [P2] The tests remain green when all reference-math timeout enforcement is deleted — fbench/src/timing.rs:161-179, fbench/src/main.rs:1052-1066, fbench/src/shared.rs:310-425. Both new timeout tests supply a fake check that calls expired() itself; neither exercises the cancellation branches that protect the reported pathological path. Reproduction performed: remove all six newly added timeout branches from mul, mul_adj_b, gram_residual, and gram_adj_residual, then compile and run the actual timing/shared module tests in an isolated harness; both baseline and mutant reported 5 passed, 0 failed. The main timeout test likewise calls the clock directly rather than these helpers. Add an armed-deadline test against the real reference functions that fails when their polls are removed.

Unverified claims

  • The green checks do not establish the claimed Rust test/clippy results — .github/workflows/ci.yml:22-50. Inspect runs 42589868038 and 42589861855: both target 8c659f1aff89c19fd379bc9e031ab8eede59168c, so they are not stale, but both execute the Python/content-only index job; neither builds nor tests fbench. The PR’s “10 passed” and clippy claims therefore depend on local evidence whose underlying logs I cannot read from the supplied artifacts. The isolated reproductions above are not a substitute for that release/feature configuration.

  • The numerical narrative lacks linked raw evidence and overstates the pasted timing ratio — docs/reviews/20261109-cell-timeout.md:55-59, docs/reviews/20261109-cell-timeout.md:81-85, and the PR body’s “15x” claim. Summing the four pasted 512-cell rows gives 406.471178 ms timed and 5237.602020 ms checked, or 12.8855× overall; individual ratios range from 9.7297× to 16.7369×, not uniformly 15×. The design note also describes the 1024 rows as timeout rows, whereas the pasted CSV contains two timeouts and two successful F-order rows. The underlying red/green run logs, build identities, and 27.6-hour campaign evidence are not linked there and I cannot read them.

Checked and fine

none

Not checked: the full release/vendor matrix or publication pipeline end to end; timeout enforcement remains bypassable in the reproduced paths.


Reviewed head: 8c659f1aff89c19fd379bc9e031ab8eede59168c at 2026-11-09T09:20:06Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Review items addressed in 6497526:

  • P1 / clock not on the executing thread — the clock is thread-local, and with_team/with_lock run the closure on a team member or worker, so arming it in run_cell armed a thread that never polled it. measure_row now arms, measures, reads CUT and disarms inside that closure (measure_armed), and returns the cut flag; the caller passes it to Cell::stopped_with instead of re-reading its own thread-local. The verification pass, which does run on the caller, arms there.
  • P1 / a successful stage past the limit still ok — timing::measure now polls after the last repetition and after the check, and returns Err in both cases, so an over-limit cell is a timeout row and its samples are discarded. The verification pass polls after check too, which it did not.
  • P2 / verification timed_ms included prepare — the verification stopwatch now starts after prepare returns, so both CSV modes time the single call.
  • P2 / tests green with the enforcement deleted — shared::an_expired_limit_stops_the_reference_math_before_it_writes arms an expired limit and asserts mul, mul_adj_b, gram_residual and gram_adj_residual return the cut; deleting any of their polls makes it assert an ordinary value. timing::a_successful_stage_past_the_limit_is_not_a_measurement fails if either new post-stage poll is removed. main::a_multithreaded_row_reads_its_clock_on_the_worker runs a 1.2 s cell at a 1 s limit through spindle::with_lock and requires timeout.

What I could not verify, and why: the harness does not build from the committed Cargo.lock with plain cargo. The lock resolves t4a-faer 0.33.0, which split Par and ParShape, while backend.rs still passes parmod::shape(..) (a Par) where a ParShape is expected and still calls Par::rayon; 21 errors, all in backend.rs, none in the three files this commit touches (type-checked only, with 0.32/0.30/0.33 all failing the same way). The supported build is scripts/build_for_faer_rev.sh with the fork and upstream checkouts, which I have not run here. So this does not merge on my word: it needs one campaign build there. I opened a separate issue for the lock/API drift so this PR is not carrying a repo-wide build fix.

@shinaoka shinaoka self-assigned this Nov 9, 2026
The committed `Cargo.lock` was a path-patched resolution -- `t4a-faer` and
`t4a-faer-traits` carried no `source`, because it was written by a measured
build that patched them to local checkouts -- so `cargo build --locked` refused
it before compiling anything. Re-resolve it from the registry at the pinned
fork's version (0.28.0).

With a real resolution the harness still did not compile, but not because
`backend.rs` is behind the API: the default features selected the pre-0.25 fork
generation (`Par` is `Copy`, `Par::rayon`, workspace queries take a `Par`)
against a crate that has the `ParShape` + team API, and that is the 21 errors.
The newest-generation branches compile unchanged against `t4a-faer` 0.33.0, so
the defaults now name the generation the locked crate has; a measured build
reads it from the checkout it links and passes `--no-default-features` plus its
own detection, which leaves its feature set byte-identical to before.

CI gains one job, `harness`, that runs `cargo build --locked` against the
registry arm -- the one configuration nobody records, because a recorded build
links the checkouts by path. `--locked` is the point: the lock is what a reader
gets, so a lock the harness cannot compile against fails there and not nowhere.

tests/test_harness_build.py pins the three parts: the lock is a registry
resolution, the defaults are the generation the locked fork has, and the
measured build does not inherit them. Two documents stop claiming the harness
does not exist and that CI has no build job.
@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 3
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • Expensive reference calculations still run with no deadline armed — fbench/src/main.rs:662–689 constructs operations before entering run_cell; Cholesky construction calls the cubic spd → mul_adj_b path at fbench/src/backend.rs:710–713,849, and LAPACK GEMM constructs its entire reference product at fbench/src/lapack.rs:895–907. Reproduce: run fbench run --threads 1 --n 4096 --family cholesky --dtype c64 --rows faer-fork --reps 1 --prime-ms 0 --cell-timeout 1 --csv /tmp/cholesky.csv, placing breakpoints on the constructor’s mul_adj_b and timing::arm; the cubic product executes before the first arm, so its polls cannot enforce any limit. The same serial-reference bottleneck can therefore consume the campaign before timeout handling becomes active.

  • A timeout does not stop subsequent library calls within a batch — fbench/src/timing.rs:98–107 polls around an entire op.run, but the harness-owned loops at fbench/src/backend.rs:364–369,1152–1169 and fbench/src/lapack.rs:939–952 invoke every batch member without polling between calls. Reproduce: arm a 5 ms deadline and pass measure an op_from whose run callback performs four iterations, each sleeping 20 ms and incrementing a counter; all four iterations execute before the post-run poll rejects the cell. This exceeds the documented limitation of waiting for the one library call already in progress: new calls continue starting after expiration.

  • Preparation can cross the deadline without a poll before further work or successful completion — fbench/src/timing.rs:91–100,114–118 and fbench/src/main.rs:925–929 omit the post-prepare boundary check. Reproduce: create an operation whose second prepare sleeps 1.2 seconds, whose other callbacks immediately return Ok, and call run_cell with run=true, one repetition, no priming and a one-second limit; the final preparation crosses the deadline, but measure returns successfully without setting CUT, allowing an ok row. Conversely, sleeping in the first preparation still lets execution enter run after expiration in both modes. This is reachable beyond synthetic callbacks: householder_apply preparation performs QR factorizations at fbench/src/backend.rs:613–620.

Unverified claims

  • The claimed “13 passed” Rust result is not established by the green checks — .github/workflows/ci.yml:35–37 runs only cargo build --locked; it neither compiles nor executes the Rust regression tests. Reproduce the evidence check: inspect gh run view 42600213451 --repo tensor4all/faer-benchmark --log; this run belongs to head ef4bf4ed and contains the build plus Python tests, not cargo test. The claimed local Rust-test transcript cannot read from the submitted artifacts; green CI does not substantiate timeout behavior.

  • The empirical timing and unchanged-number claims lack inspectable run artifacts, and the reported timeout count conflicts — docs/reviews/20261109-cell-timeout.md:57–60,71–72,81–85 asserts measured costs, unchanged recorded numbers and the end-to-end outcome, but the underlying raw runs cannot read from the diff. Furthermore, line 84 describes the n=1024 rows as timed out, while the supplied PR CSV contains two timeout rows and two successful F-order rows at that size. Reproduce the numerical claim with the stated release invocation and retain its CSV/log; establishing unchanged numbers additionally requires a matched baseline/candidate comparison, which the diff does not provide.

Checked and fine

fbench/src/main.rs:755 places the diagnostic columns after profile; pinned campaign_toolkit/timing_evidence.py:85,144 rejects timeout statuses.

Not checked: full Cargo/vendor suites or real campaign reruns; the reproduction cases above are source-derived, not full-harness executions.


Reviewed head: ef4bf4ed77bce6e21d7c7e151d9d83153e0a9b60 at 2026-11-09T10:26:51Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 4
UNVERIFIED: 1

Critical (must not merge)

none

Needs changes

  • The timeout is unarmed during cubic input/reference construction — fbench/src/main.rs:665–690, fbench/src/backend.rs:710–713, fbench/src/lapack.rs:895–907: ops_for() runs before run_cell() arms the deadline, so Cholesky input generation and the vendor GEMM reference product can consume unlimited time despite containing the newly added polls. Reproduce: run fbench verify --threads 1 --n 4096 --family cholesky --dtype c64 --rows faer-fork --cell-timeout 1 --csv /tmp/cholesky.csv; trace backend::spd → shared::mul_adj_b before the first timing::arm call. Throughout that construction, expired() sees no deadline. Killing the process there still produces neither a timeout row nor the completed rows held in memory.

  • Batch execution continues launching library calls after the deadline — fbench/src/timing.rs:98–100, fbench/src/backend.rs:364–369, fbench/src/lapack.rs:938–952: the timing loop polls around an entire Op::run, but these harness-owned batch loops do not poll between individual library calls, so overrun is not limited to the one call already in progress. Reproduce: construct an op_from whose run closure loops four times, sleeping 20 ms per iteration and incrementing a counter; arm a 5 ms deadline and call measure(..., 0, 1, ...). The isolated probe executed all four iterations before returning the timeout error. Increasing batch extends the unchecked work.

  • Preparation can exhaust the budget without preventing another expensive call, and final preparation can produce an over-budget ok row — fbench/src/timing.rs:97–100,117–118, fbench/src/main.rs:925–929: neither execution mode polls between successful preparation and run, and the timing path returns success immediately after its final preparation. Reproduce: with a 5 ms deadline, make the first preparation sleep 30 ms and count executions of run; the probe still executed run after preparation exhausted the budget. Separately, make only the second preparation sleep 30 ms, with instantaneous run/check closures: the probe returned Ok with cut() == false after 66 ms, which run_cell accepts as status=ok.

  • The committed design document contradicts both the implementation and the supplied evidence — docs/reviews/20261109-cell-timeout.md:25–26,52–53,83–84: it specifies a process-wide Instant/AtomicU64 pair instead of thread-local cells, omits profile from the trailing CSV layout, and describes the n=1024 rows as timeouts although the PR’s quoted CSV contains two timeouts followed by two successful rows. Reproduce: compare those passages with fbench/src/timing.rs:14–21, the header at fbench/src/main.rs:755, and the eight CSV rows in the PR body.

Unverified claims

  • The numerical campaign claims and measurement invariance lack independently readable evidence — docs/reviews/20261109-cell-timeout.md:58–60,71–72,81–85: cannot read the cited measuring-host CSVs/logs, and the diff does not establish the 27.6-hour history, the reproduced campaign timings, or that existing recorded numbers remain unchanged. Verification procedure: retain before/after artifacts with library and harness identities and rerun the supplied command on the measuring host. The quoted four successful n=512 rows establish check/timed ratios of approximately 15.1×, 16.7×, 9.7× and 9.8×, or 12.9× in aggregate, rather than a uniform 15×.

Checked and fine

  • Both inspected successful CI runs identify head ef4bf4ed77bce6e21d7c7e151d9d83153e0a9b60; they are not stale-head evidence.
  • .github/workflows/ci.yml:36–37 runs cargo build --locked, not Rust tests; green does not exercise the timeout assertions.
  • Not checked: full Cargo/vendor builds, mutation suite, or campaign reruns; the isolated probes do not establish end-to-end campaign behavior.

Reviewed head: ef4bf4ed77bce6e21d7c7e151d9d83153e0a9b60 at 2026-11-09T10:54:25Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Review at ef4bf4e is NEEDS 4; all four are real and none is a wording problem. Plan:

  1. Arm before anything expensive. The deadline is armed inside run_cell, but ops_for()
    (Cholesky input build, vendor GEMM reference) runs first. Arm at the top of the cell, before
    ops_for, so construction polls too.
  2. Poll inside the harness-owned batch loops. backend.rs:364-369 and the vendor batch loop in
    lapack.rs call the library several times within one Op::run; they need a poll between
    iterations, not only around the whole run.
  3. Poll between prepare and run. Both modes return after a successful prepare; a prepare that
    exhausts the budget must cut the cell, and the final prepare must not produce an ok row.
  4. Rewrite docs/reviews/20261109-cell-timeout.md to the thread-local design that landed, the
    trailing profile,timed_ms,check_ms layout, and the actual CSV rows (two timeouts then two ok),
    and drop the uniform "15x" phrasing for the per-row ratios.

Each will carry an executed counterexample (the probes in the review are the shape I will use), and
the numeric claims will be restated from the CSVs in the PR rather than a summary. Not merged in
the meantime.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant