From 607da3ce99caada198aa374f7d35cc147a416c2e Mon Sep 17 00:00:00 2001 From: logan-nc Date: Sat, 12 Sep 2026 10:42:44 -0400 Subject: [PATCH] Repo - DOCS - Record what diff-scoped review cannot see about performance Optimizing the NTV bounce-point search removed a hot path that had cost order 10% of total runtime while sitting untouched in KineticForces. Three things let it survive, and none of them were the size of the pull requests. Every agent in the review pipeline is diff-scoped. The call was pre-existing code that no KineticForces PR modified, so it was never in a diff and never handed to julia-performance-optimizer -- which is separately told to go straight to a named function and not profile the suite, so it could not have found it regardless. The README listed a performance reviewer without saying what it cannot see. Profilers frame the problem and reviewers optimize inside that frame. The hot line called a root solver, so every proposal was to make root-finding cheaper; nobody asked whether a root solver belonged there, when the interpolant is a cubic and a cubic's level set is a closed form. That question comes from naming the mathematical object, not from a profile. The minimal-change rule read against the fix: a closed-form cubic solver is new lines, and "don't write new general-purpose machinery" argues against writing it. It needed a carve-out for new lines that remove a solver. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0186CLfjeJeSusNTva5RMfGU --- .claude/agents/README.md | 10 ++++++++++ .claude/agents/julia-performance-optimizer.md | 20 +++++++++++++++++++ CLAUDE.md | 1 + 3 files changed, 31 insertions(+) diff --git a/.claude/agents/README.md b/.claude/agents/README.md index 11346fdae..94a4c31e8 100644 --- a/.claude/agents/README.md +++ b/.claude/agents/README.md @@ -23,6 +23,16 @@ Run sequentially, reading each agent's findings before launching the next: 3. **`julia-performance-optimizer`** and/or **`fast-interpolations-optimizer`** — only if the change is performance-relevant. 4. **`regression-guardian`** — always, last, before merge. Confirms the numbers didn't silently move. +**These agents are diff-scoped, and that leaves one thing uncovered.** Each reviews what changed, so a +hot path nobody is editing is invisible to all of them — permanently, no matter how expensive it +becomes. `julia-performance-optimizer` in particular is handed a named function and told not to +profile the suite, so it cannot find a hotspot outside the diff and should not be described as though +it had. Whole-program cost is answered only by profiling a full run, which is a developer-initiated +check rather than anything the pipeline does on its own. Two moments are worth spending it on: when a +module first lands in a hot path, and when someone's sense is that runtime has changed a lot. When +asked to do performance work, do not assume the diff is the scope — say what the whole-run cost +picture is, or say that it is unmeasured. + Not every change needs all four. A docs-only change needs none; a pure perf refactor still needs the physics reviewer (to confirm no numerical change) and the regression-guardian. ## Budget diff --git a/.claude/agents/julia-performance-optimizer.md b/.claude/agents/julia-performance-optimizer.md index 189de57c7..26252655a 100644 --- a/.claude/agents/julia-performance-optimizer.md +++ b/.claude/agents/julia-performance-optimizer.md @@ -43,6 +43,26 @@ You operate under a hard budget to protect the user's token quota: - For major optimizations, recommend adding benchmark scripts to test/ directory - Compare performance metrics (time, allocations, memory) between original and optimized versions +## Before optimizing: cost the work, don't just locate it + +A profiler says where time goes. It never says what the work *should* cost, and optimizing inside its +framing yields a faster version of an operation that should not exist. Answer both before proposing +any micro-optimization: + +1. **What is the mathematical object?** Name the operation and what its data structure already + determines. A level set of a piecewise cubic is a root *formula*, not a search; an integral of a + spline is a closed form. If a general-purpose solver is being called on a structure with an exact + solution, replacing the solver *is* the optimization and everything else is polishing. +2. **What is the cost per work item?** Divide the profile's share by the number of items (calls × + roots × evaluations) and compare against a first-principles estimate. A ratio of 10× or more means + the algorithm is wrong, not the constants — report that and stop, rather than shaving constants. + +Read the **enclosing loop for invariants a profile cannot show**. If an outer loop advances +monotonically, each iteration's answer is a hint for the next and a blind search is waste — the +codebase already uses this idiom through FastInterpolations' `hint=` arguments. Report allocations per +call for any loop running more than ~10³ times, and say plainly when an optimization buys exactness +rather than speed. + ## Workflow When presented with code to optimize, follow this structured approach: diff --git a/CLAUDE.md b/CLAUDE.md index 9417068b5..7f80f243a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,6 +111,7 @@ Roster, the recommended review pipeline (physics fidelity → readability → pe ### Minimal-change discipline - **Reuse native ops and existing utilities before writing new ones.** FastInterpolations splines integrate and differentiate natively (`integrate`, `cumulative_integrate`, `deriv1`); the Equilibrium module already has flux-surface integration/average patterns. Do not reimplement spline integration, quadrature, or differentiation — grep for the existing idiom first. - **Size the change to the problem.** A small numerical correction (e.g. a ~1% fix) should be a handful of lines, not new general-purpose machinery. Resist faithfully porting Fortran scaffolding (custom integrators, power-law spline bases) when a native call plus a one-line correction gives the same numbers — verify equivalence instead of assuming the elaborate version is needed. +- **Replacing a solver with its closed form is a reduction, not new machinery.** The rule above targets ported scaffolding, not exact methods. Where the data structure already determines the answer — a spline cell is a cubic, so a level set of it is a root formula and the stationary points are roots of a quadratic — writing that solution and deleting the iterative solver removes machinery even when the line count rises. Justify it with the residual, not the line count. - **Don't commit throwaway artifacts for minor fixes.** No in-repo benchmark scripts/outputs or agent-memory churn for a small change — these accumulate and outsize `src`. Verify with a scratch script (e.g. under `/tmp`) and the regression harness; the regression harness is the durable record of numerical behavior. ### Output Files