diff --git a/AGENTS.md b/AGENTS.md index 7ef3403e..876db3f7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,6 +31,33 @@ rule files the task needs. * Every experiment carries one focused test. Run it locally; the fork CI gate is a build, not the full upstream suite (faer's test dependencies are heavy). The inherited upstream workflows still run their own heavier jobs. -* When an experiment uncovers a likely upstream bug, report it to the fork - maintainer and get explicit permission before preparing or opening an upstream - issue or PR. +* Upstream feedback is **deferred at this stage**: do not prepare or open an + upstream issue or PR and do not write to an upstream tracker, even for a defect + that is clearly upstream's. Measure it, record it in `docs/worklogs/` with the + upstream revision that was measured, and leave the decision to the maintainer. + Explicit permission is required before anything more than a record here. + +## Work Logs And Review Intent + +Follow the shared [Work Logs And Design +Records](https://github.com/tensor4all/tensor4all-agent-rules/blob/main/rules/common/repository.md#work-logs-and-design-records) +policy and the [local format](docs/worklogs/README.md). Keep a record for a +nontrivial multi-phase change, a non-obvious design choice, or a performance +experiment; small fixes and AI assistance alone do not require one. One file per +change theme under `docs/worklogs/`, linked from its PR, and read the linked +record when reviewing the decisions it explains. + +Every performance experiment in this fork needs one, because the fork's whole +purpose is to be a measurable delta against upstream. `PATCHSET.md` says what the +fork carries today; the record says why — the isolation, the candidates that were +tried and dropped *with the measurement that dropped them*, and what the result +establishes. It is how the difference from upstream and the course of an +optimisation stay followable by someone who was not in the session, and how a +dead end that cost a day is not tried twice. + +Keep complete performance results in the linked evidence (the campaigns' +published pages) rather than copying them into the record, and say plainly when a +number is a scan rather than a paired comparison, or when something is a +hypothesis rather than a measurement. A lead nobody has measured yet goes under +"what remains unverified" in the record of the experiment that uncovered it — the +ledger lists what the fork *carries*, not what it suspects. diff --git a/PATCHSET.md b/PATCHSET.md index f80fbe61..981f80d1 100644 --- a/PATCHSET.md +++ b/PATCHSET.md @@ -28,7 +28,7 @@ upstream `faer`. | EVD eigenvector normalization + overflow-safe back substitution | `evd_real`/`evd_cplx` return eigenvectors scaled to unit 2-norm (a conjugate pair is scaled as one complex vector, matching `xGEEV`), and the back substitution is rescaled when an entry would exceed `sqrt(max)` instead of overflowing to `NaN`. A finite input that still overflows, or a Schur iteration that does not converge, returns `NoConvergence`. | KEEP | tenferro-rs#2131 | Focused test `eig_2131_tests` (f32/f64 Jordan block, conjugate pair with left and right vectors, complex input, sequential + recursive solver). Normalization runs at the `evd_real`/`evd_cplx` boundary, so `operator/eigen` keeps its previous Ritz-vector scaling while still getting the overflow-safe finite Schur eigenvectors. | | Unpivoted QR column skip | The skip lived only in `qr_in_place_unblocked`; `qr_in_place_blocked` has no threshold of its own and recurses into that core, so removing it there covers the tall and wide paths at once. The reflector is applied and the row advances for every nonzero column, matching LAPACK; the tall/wide `svd` path goes through it too. | KEEP | tlinalg-rs#28, tensor4all-rs#836 | Adopted. Tests: `test_near_dependent_columns_are_not_skipped` (tall `m x 3`, m=100/1000/8192) and `test_near_dependent_rows_are_not_skipped` (its transpose, wide `3 x n`, n=100/1000/8192), both at `<=1e-13`; measured 1e-16 to 5e-15. The wide fixture was already accurate before the change: at m=3 the removed threshold `16 (m-k) eps ||column||` is ~1e-14 and never tripped it (a wide skip's relative reconstruction error is ~`16 eps sqrt(m) sqrt(m/n)`), so the wide path needed no separate treatment — the same core change covers it. tlinalg's reported wide `3.33e-7` is a reconstruction-indexing bug in that test (entry `(i,j)` written to flat `j+3i` while the factor is column-major `i+3j`); faer's factors for that matrix are accurate to ~4e-16. Existing columns are unchanged; formerly skipped nonzero columns now pay reflector/update flops (the threshold norm bookkeeping is removed). No existing benchmark suite was run. Release smoke, 3-run mean, before→after: 100000×8 2.984→2.986 ms; 1000×1000 19.793→19.921 ms. | | Row-split reduction in the compact-WY factor's deep half | `upgrade_householder_factor`'s flat branch (`prev_block_size < 8`) builds `T = diag(tau) + strict_upper(V^H V)`, and its second product reduces over every row below the basis' diagonal with an `n x n` output -- the same shape as the block apply's reduction, and serial for the same reason: 0.271 ms at eight threads at `m = 4096, n = 16`, against 0.040 with the rows split (6.7x; 2.96x at `m = 512`, 4.59x at `m = 1024`). The partials here are `n x n` each, tiny against the basis, and only the strict upper triangle is stored, so the split's overhead is negligible and no depth-against-width gate is needed -- the thread and work floors are enough. End to end, 8T pool: `householder_factor` 4096x256 b1 c64 15.72 -> 13.38 ms (f64 5.63 -> 4.84), `qr` c64 42.76 -> 37.00, and the 512x512 and 256x256 cells improve too (4.49 -> 4.14, 0.98 -> 0.92) with every 1T row unchanged. Test: `threaded_upgrade_matches_the_sequential_one` (4096x16, 2048x8, 4096x1, and 64x8 below the floors). | KEEP | tlinalg-rs#46 | Costs one `n * n * threads` buffer per call; the same scratch note as the row below. Own code: no OpenBLAS or MKL source. | -| No parallel region below the compact factor's own floor | `upgrade_householder_factor` threads its `V^H V` products, and its `deep` predicate already states when the reduction has the shape to pay for that (deeper than `2 * n`, at least one row per thread, over the `8 * 1024` work floor). Below that floor the products still opened a region each, and a blocked QR calls this function once per block -- at a block size the recursion halves every level -- so a `128 x 128` QR paid the fork-join cost of `n`-wide reductions about fifty times for work that same floor calls too small to split: the threaded call measured 4.2-4.6x *slower* than the sequential one at `n = 128` (1.05 vs 0.23 ms), 2.1x at 256, 1.3x at 384, and only broke even at 512 (0.96). Asking the floor once, before opening anything, gives 0.99 at 128, 0.79 at 256, 0.60 at 384 and 0.44 at 512 (f64), 0.72 / 0.50 / 0.30 (c64), and no region is opened below the floor at all. The tall shapes the ledger's other rows serve are untouched (`4096x256` f64 6.35 -> 6.33 ms, c64 16.67 -> 16.67), because their reductions are above the floor; `1024x64` goes from 1.4x slower than sequential to parity. No new constant: the predicate is the one the products below already use. | KEEP | tlinalg-rs#46 | Measured with an isolated driver on the call tlinalg's `qr` family makes (`qr_in_place` f64/c64, 128-768 square, 4096x256 / 1024x64 / 512x512), CPUs 4-11, best of 7. | +| No parallel region below the compact factor's own floor | `upgrade_householder_factor` threads its `V^H V` products, and its `deep` predicate already states when the reduction has the shape to pay for that (deeper than `2 * n`, at least one row per thread, over the `8 * 1024` work floor). Below that floor the products still opened a region each, and a blocked QR calls this function once per block -- at a block size the recursion halves every level -- so a `128 x 128` QR paid the fork-join cost of `n`-wide reductions about fifty times for work that same floor calls too small to split: the threaded call measured 4.2-4.6x *slower* than the sequential one at `n = 128` (1.05 vs 0.23 ms), 2.1x at 256, 1.3x at 384, and only broke even at 512 (0.96). Asking the floor once, before opening anything, gives 0.99 at 128, 0.79 at 256, 0.60 at 384 and 0.44 at 512 (f64), 0.72 / 0.50 / 0.30 (c64), and no region is opened below the floor at all. The tall shapes the ledger's other rows serve are untouched (`4096x256` f64 6.35 -> 6.33 ms, c64 16.67 -> 16.67), because their reductions are above the floor; `1024x64` goes from 1.4x slower than sequential to parity. No new constant: the predicate is the one the products below already use. | KEEP | tlinalg-rs#46 | Measured with an isolated driver on the call tlinalg's `qr` family makes (`qr_in_place` f64/c64, 128-768 square, 4096x256 / 1024x64 / 512x512), CPUs 4-11, best of 7. Why, and the candidates that lost: `docs/worklogs/2026-10-14-compact-factor-region-floor.md`. | | Row-split reduction in the block Householder apply | `tmp = V^H C` has an `N x K` output and reduces over every row of the target. With `N` small and the target long, the products inside the generic path were one serial reduction inside one task, so the column split (capped at `min(par, K, 4)`) could not use the depth: 4096x16 basis against a 4096x240 target measured 2.03 ms at 8T where OpenBLAS's `?gemm` on the same shape is 0.53-0.71. Splitting the rows is a split along the depth -- it adds `threads` partials of `N x K` to write, read and add -- so it is taken only while the reduction is *deeper than it is wide*: a sweep over `M` against `K` at eight threads puts the crossover at `M ~ 2 K` (four times deeper gains 3-4x; as wide as it is deep, which is the 512x512 trailing update at `M = 512`, `K = 496`, loses for the partials it pays for). With that gate, 8T pool, `householder_factor` 4096x256 b1 c64 17.44 -> 15.72 ms (f64 5.83 -> 5.54), `qr` c64 46.75 -> 42.68, `svd_thin` c64 77.11 -> 68.16, and the 512x512 and 256x256 cells stay at baseline; every 1T row is unchanged. Tests: `threaded_panel_matches_the_sequential_one` (2048x4, 4096x16, and two shapes below the thresholds) plus the two existing cases. | KEEP | tlinalg-rs#46 | Costs one `N * K * threads` buffer per call on the new path; the public scratch function is unchanged, so upstreaming this should carry it there. Own code: no OpenBLAS or MKL source is used or adapted. | | Threaded rank-one Householder apply | The `N == 1` arm of `apply_block_householder_on_the_left_in_place_generic` is a sequential rank-one update per target column and it never read `par`. A tall blocked QR's panel is made almost entirely of those updates, so the panel was the serial part of it: at 4096x256 c64 the panel is 5.96 ms at 1T and 6.31 ms at 8T, against OpenBLAS's `?gemv`/`?geru` panel at 5.97 -> 2.58 ms on the same eight cores. When the caller asks for threads and the update is at least 8192 elements, the arm now falls through to the generic path, which partitions the target's columns and threads its two products. Through tlinalg's compact factor (`householder_factor`), pool row, 4T / 8T: 4096x256 f64 6.73 -> 5.77 and 6.67 -> 5.83 ms, 4096x256 c64 21.42 -> 18.75 and 19.50 -> 17.44 ms (11-14%), with 512x512, 128x128, `qr`, `svd_thin` and every 1T row unchanged within noise. Tests: `threaded_rank_one_matches_the_sequential_one` (f64 and c64; 2048x8, 16384x1, 64x8) and `rank_one_negates_its_own_reflector_column`. | KEEP | tlinalg-rs#46 | Own code: no OpenBLAS or MKL source is used or adapted, their timings are only the target. | | Tall compact-WY trailing update scaling | The blocked compact QR applies its panel through `apply_block_householder_transpose_on_the_left_in_place_with_conj`, whose generic path partitions the target's columns with `n_tasks = min(parallelism_degree(par), K, 4)` and `rayon(par / n_tasks)` inside each. On a tall DRAM-bound panel (4096x16 basis, 4096x240 target, eight physical Zen 5c cores sharing 8 MiB of L3, 16.8 MB target) the update is 8.72 ms at `Par::Seq` and 2.45 ms at `Par::rayon(8)` = 3.56x, where OpenBLAS reaches 5.5x on the same cores for the same factorization; splitting the columns by hand saturates at 3.87x, so the ceiling is the kernel, not the decomposition. Candidates: partition rows for the `C -= V tmp` half, which is row-parallel and has the long dimension (rows >> columns, while the column partition caps at 4), and the DRAM-bound path of the inner matmuls. | TODO | tlinalg-rs#46 | Reproducer and the phase split: tlinalg-benchmark `docs/worklog.md`, 2026-10-12. Drives no code change in tlinalg until the kernel moves. | diff --git a/docs/worklogs/2026-10-14-compact-factor-region-floor.md b/docs/worklogs/2026-10-14-compact-factor-region-floor.md new file mode 100644 index 00000000..9b57e45c --- /dev/null +++ b/docs/worklogs/2026-10-14-compact-factor-region-floor.md @@ -0,0 +1,78 @@ +# The compact factor's parallel region floor + +PR: [tensor4all/faer#9](https://github.com/tensor4all/faer/pull/9) (merged as +`04542fae`). Ledger row: `No parallel region below the compact factor's own floor`. +Consumer: [tensor4all/tlinalg-rs#56](https://github.com/tensor4all/tlinalg-rs/pull/56). + +## Decisions + +- **Ask the floor once, before opening anything.** `upgrade_householder_factor` + threads its `V^H V` products, and its own `deep` predicate already states when that + reduction has the shape to pay for it: deeper than `2 * n`, at least one row per + thread, over the `8 * 1024` work floor. Below that floor the products still opened a + rayon region each — and a blocked factorisation calls this function **once per + block**, at a block size the recursion halves every level. The change is: below the + floor the call takes `Par::Seq`, above it the caller's `par` is forwarded exactly as + before. Thirteen lines, no new constant, nothing computed differently. +- **Why it was worth changing.** A threaded 128x128 QR measured **4.2-4.6x slower** + than the sequential call on the same input — the fork-join cost of `n`-wide + reductions paid about fifty times for work the same floor calls too small to split. + Isolation: `qr_in_place` (the call tlinalg's `qr` family makes), f64/c64, + CPUs 4-11, best of 7. `Par::rayon(8)` before → after, ms: 128x128 1.05 → **0.235**, + 256x256 2.46 → **0.917**, 512x512 4.59 → **2.85**, 1024x64 0.65 → **0.466**, + 4096x256 6.32 → 6.31 (unchanged, its reduction is above the floor). + At tlinalg level the pair of campaign pages `bf88c0a6e568-mkl` → `7fdba8a53ba3-mkl` + gives `faer-pool` medians of **0.576** (f64) / **0.742** (c64) for `qr`, with the + sequential rows and the unchanged MKL binary at 1.00 as the floor; `qr` 128x128 b128 + c64 goes 121.77 → **40.79** ms against MKL's 39.29 (3.10x behind → **1.04x**). +- **Rejected: gate the per-block apply on the trailing area** + (`(m - row) * (n - col) < par_threshold`, the rule the column-pivoted sibling uses + for itself). It fixes the small and medium shapes and **regresses the tall ones**: + 4096x256 f64 6.36 → 6.82 ms, c64 16.53 → 18.23. A tall block update is long in depth + and small in area, so an area threshold sends real wins to `Par::Seq` — the same + reason the fork's own block-apply gate is a depth-against-width inequality and not + an area. +- **Rejected: force the `n x n x n` head product of the `prev_block_size < 8` branch to + `Par::Seq`.** 512x512 f64 2.49 → 3.43 ms (38% worse), neutral elsewhere. +- **Interacts with** the fork's rows `Row-split reduction in the compact-WY factor's + deep half` and `Row-split reduction in the block Householder apply`: those splits are + *inside* this function and stay exactly as measured; this change is the gate in front + of them, so a shape that used to open one region now opens none, and a shape above the + floor behaves as before. +- **Upstream.** Upstream's `upgrade_householder_factor` forwards `par` to the same + products and the same `join_raw` recursion with no floor at all, so the pattern is + upstream's, not the fork's; upstream's `qr/no_pivoting/factor.rs` likewise declares a + `par_threshold` that nothing reads. The fork cannot port the fix back verbatim, because + the floor it reuses is itself a fork addition from the row-split work. + *What was measured* is the published upstream release, `faer-0.24.4` from crates.io + (linked by path from the registry source), with the same driver, host and pins as the + fork's numbers on this page. *What was read* is the GitHub mirror clone + `sarah-ek/faer-rs` at `ddbd2ae`; the source of truth is Codeberg, so treat the line + numbers there as the mirror's, not upstream's. Upstream feedback is deferred for now + (see `AGENTS.md`). + +## Verification conclusions and constraints + +- Establishes: a threaded no-pivot QR is at worst at parity with the sequential call and + wins from `n = 224` upward (0.88x at 224, 0.37x at 768), and the tall shapes the + ledger's other rows serve are unchanged within 1.2%. +- Limits: the tlinalg-level pair is a **scan**, not a paired comparison (`--aa 1`), so no + pooled cell is claimed; one pooled cell moved the wrong way and is not attributable + (`householder_apply` 32x32 b128 f64 reads 0.246 → 0.472 ms, a shape that fans the batch + out over lanes with a sequential inner call and therefore does not contain this change + at all). One pre-existing failure in the fork is unrelated: + `sparse::linalg::qr::tests::test_numeric_simplicial_qr_1_transpose` panics in + `triangular.rs:990` on the parent commit too. +- Remains unverified, and these are hypotheses only — each was found by reading the + source while measuring this change, none has an isolated measurement yet: + - `svd/bidiag.rs`: `bidiag_in_place_scratch` discards its hint (`_ = par; _ = + params;`) and `BidiagParams::par_threshold` is never read, so the bidiagonalisation + is sequential however the caller calls. Consistent with `svd_values` being flat from + 1T to 8T (128x128 b1 f64 1.412 → 1.433 ms against MKL's 0.704 → 0.678). + - `evd/tridiag.rs` uses no parallel degree at all, and `eigh` is flat for the same + shape (1.26 → 1.24 ms against MKL's 1.08 → 0.586). + - `lu/partial_pivoting/factor.rs` is *not* missing its hint (panel solve, trailing + matmul and recursion all take `par`, and its `par_threshold` is read), but + `lu_factor` 512x512 b1 f64 scales only 1.85x from 1T to 8T where MKL reaches 4.9x. + This one needs a phase profile before any threshold is touched; the `M ~ 2K` episode + in this ledger is the lesson about choosing a constant from the wrong cell. diff --git a/docs/worklogs/README.md b/docs/worklogs/README.md new file mode 100644 index 00000000..4b66cc4c --- /dev/null +++ b/docs/worklogs/README.md @@ -0,0 +1,31 @@ +# Work Logs + +Keep lightweight records for nontrivial multi-phase changes, non-obvious design +choices, or performance experiments, following the shared +[Work Logs And Design Records policy](https://github.com/tensor4all/tensor4all-agent-rules/blob/main/rules/common/repository.md#work-logs-and-design-records). +Small fixes and AI assistance alone do not require one. + +One file per change theme: `YYYY-MM-DD-.md`, linked from the PR that carries +the change. Start with a few lines: + +```markdown +# + +## Decisions +- Chosen approach and why; important rejected alternatives, if any. + +## Verification conclusions and constraints +- What the final state establishes, and what remains unverified or limited. +- Links to detailed evidence or special reproduction conditions, when needed. +``` + +A performance experiment in this fork has one extra duty: an upstream revision was +measured, or it was not, and the record says which. `PATCHSET.md` is the ledger of +what the fork carries; a record here explains why it carries it and what lost along +the way. + +Do not include commands run (Cargo or otherwise), files read, agent activity, or +edit/review chronology. Mention failed attempts only when they explain a choice or +limitation. Keep complete performance results in the linked evidence rather than +copying them here. Update the same record when its conclusions change, not after +every correction; do not rewrite historical records for this format.