Repository navigation
Repo - DOCS - Record what diff-scoped review cannot see about performance - #459
Merged
Merged
Conversation
…ance 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186CLfjeJeSusNTva5RMfGU
matt-pharr
approved these changes
Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release note
Records three things learned while optimizing the NTV bounce-point search (#456), so the next performance pass starts from them rather than rediscovering them.
Why
#456 removed a hot path that had been costing order 10% of total runtime, in code that had been sitting in
KineticForcesuntouched. Worth asking how it survived that long:Every agent in the review pipeline is diff-scoped.
Roots.find_zeroswas pre-existing code that no KineticForces PR modified, so it was never in anyone's diff and never handed tojulia-performance-optimizer— which is separately instructed to go straight to a named function and not profile the suite, so it could not have found it even if invoked. A one-line PR would have missed it identically; the size of the PRs was not the mechanism. Nothing in the pipeline looks at whole-program cost, and the README implied otherwise by listing a performance reviewer without saying what it cannot see.Profilers frame the problem, and reviewers optimize inside that frame. The hot line was a call to a root solver, so every proposal — including the first version of #456 — was "make root-finding cheaper". Nobody asked whether a root solver belonged there at all. The answer was in the type: the interpolant is a cubic, and a cubic's level set is a closed form. That question does not come from a profile; it comes from naming the mathematical object.
The minimal-change rule read against the fix. "Don't write new general-purpose machinery" is a good rule aimed at ported Fortran scaffolding, but a closed-form cubic solver is new lines, and the rule as written argues against writing it. It needed an explicit carve-out for the case where new lines remove a solver.
What this changes
CLAUDE.md— one bullet in Minimal-change discipline: replacing a solver with its closed form is a reduction, not new machinery; justify it with the residual rather than the line count..claude/agents/julia-performance-optimizer.md— a gate ahead of the existing workflow: name the mathematical object and estimate cost per work item before proposing micro-optimizations; read the enclosing loop for invariants a profile cannot show; say plainly when an optimization buys exactness rather than speed..claude/agents/README.md— states the blind spot directly, so the pipeline is not mistaken for covering whole-program cost, and names the two moments worth spending a full-run profile on.Considered and rejected
Adding a
--profilemode to the regression harness to track cost share across commits, the way it tracks numerical quantities. Rejected on coverage: to stay cheap it would profile one canonical case, anddiiid_n1is an ideal-MHD case that never entersInnerLayerorTearing— precisely the modules under active development. It would have institutionalised a blind spot while giving the impression cost was covered, and widening it to every case removes the reason it was cheap. Any rule fixed now about which paths to watch ages badly as hot paths move. A developer noticing runtime has changed a lot remains the better trigger; these changes aim at making the follow-up ask the right question.Regression report
Not applicable — no
src/changes. Instruction and agent-definition files only.Notes for reviewers
The concrete evidence behind the cost-per-work-item rule: in #456 the same bounce-point output went from 20,953 profile samples to 82 — a 256x ratio for an identical answer. Nothing in a share-of-time profile flags that; dividing by the number of work items and comparing against what the operation should cost does.
🤖 Generated with Claude Code
https://claude.ai/code/session_0186CLfjeJeSusNTva5RMfGU