docs: clones are the symptom — fact re-derivation is the disease

[briansrls]

Summary

Post dark-emu-36 rewrite of the perf design docs. The previous framing focused on `.clone()` elimination as the perf target. A real profiling session proved that framing was wrong.

What dark-emu found

The biggest win wasn't on the plan. A 6-line fix to `merge_envs` produced a 68× speedup on the reconcile stage — no clone elimination involved.

Metric	Before	After	Δ
Reconcile stage	9.54s	140ms	68×
Total pipeline (gist test)	11.72s	2.34s	5×
Self-compile	60-75s	37.6s	~2×
`.clone()` count	13,724	13,724	unchanged

The bug

PR2 threaded a single `InternTable` through every `TypeEnv`. But `merge_envs` didn't know this — it kept iterating all envs' intern tables and rebuilding a fresh one from the merged contents. Per module × 2 merges × 3 envs × every string = pure waste.

The fix:
```
let merged_intern_table = match envs |> first {
Some { value: first_env } => first_env.intern_table
None => empty_intern_table()
}
```

Because all envs shared the same upstream table, merging them is "take the first one."

The lessons (5 rules)

1. Profile first. Before any perf plan, run the profiler. Use it to INVALIDATE hypotheses.
2. Audit for re-derivation, not just clones. Look at `merge_`, `combine_`, `collect_`, `unify_`, `reconcile_`, `build_from*` — each is a candidate for "rebuilds a fact upstream already computed."
3. When you thread a fact forward, DELETE the old reconstruction. PR2 threaded InternTable but left merge_envs rebuilding it. Every "thread X through pipeline" PR must include "delete downstream reconstruction of X."
4. Watch for fact-flow violations in reviews. Functions taking N structured inputs and producing one output are re-derivation hazards.
5. This is KF-2 we commit against ourselves. `merge(a, a, a) = a` by idempotency. The compiler should reject this. It doesn't — KF-2 isn't built yet. Every perf bug we hit of this shape is advance payment on KF-2's priority.

What this means

The perf crisis was not what we thought it was. Clones are the visible symptom, not the disease. We counted 21,211 clones and assumed they were the problem. Most clones are refcount bumps (cheap). The real cost was one function doing O(n²) work on data it could have read in O(1).

We're running a half-built causal engine against its own code. KF-2 would catch merge_envs-class bugs at compile time. Until KF-2 exists, we'll keep committing them, finding them through painful profiling, and paying for the investigation time.

Files changed

• `docs/perf-borrow-design.md` → `docs/perf/clone-elimination.md` (rewritten as case study)
• `ROADMAP.md` PERF section (new framing + rules)
• `THESIS.md` (merge_envs added as case study under algebraic simplification)
• `ROADMAP.md` KF-2 section (priority elevated)

Review asks

1. Does the merge_envs framing correctly identify the pattern, or is there a deeper root cause?
2. Are the 5 rules actionable? Would you add or remove any?
3. Is KF-2 priority elevation justified, or is it a different milestone's concern?
4. The doc asks "how many more merge_envs-class bugs are hiding?" — is there a systematic audit we should commit to as a regular practice?

🤖 Generated with Claude Code

[briansrls]

ChatGPT Review

Overall, the new framing is directionally right. Grounded in the thesis, “clones are the symptom” is exactly the repo’s root-cause doctrine: the invariants say to push fixes to the deepest missing fact, and “Facts Flow Forward” says the real fix is to stop losing a fact rather than optimize the downstream compensation. On that axis, this PR is aligned with the project’s core thesis. chatgpt-review-8976254b-5675-4b…

chatgpt-review-8976254b-5675-4b…

I’d leave two review comments.

1. Medium — the PR turns session measurements into source-of-truth ratchets without updating the actual authorities.

In the diff, ROADMAP.md now says “Perf ratchet 120s → 55s” and the new perf doc says current .clone() sites are 13,724. But the checked-in test authority still has PERF_RATCHET_SECONDS = 120, and its own comment says GitHub CI runners can hit 60s+, which is exactly why the current ratchet is still 120. Separately, the test suite still tracks ownership/clone debt with an ownership-violation ratchet and a stage0 clone census ratchet, so the docs are no longer describing the same source of truth the code enforces. The invariants explicitly say metadata should have a single producer and docs should not become the authority. I would either reword these as “dark-emu session measurements” or update the tests in the same PR. chatgpt-review-b056fa51-91f3-4c…

chatgpt-review-b056fa51-91f3-4c…

chatgpt-review-8976254b-5675-4b…

2. Medium — the rewrite over-corrects from “clone count wasn’t the hotspot in this profile” to “clone work is no longer a priority metric,” which drifts from the active roadmap.

The current roadmap still treats LS-4 / Stream B clone-elision as active work, with three explicit layers, and says borrow propagation is where roughly 90% of unnecessary clones live. The test suite also still treats ownership-vs-emission clone mismatches as real tracked debt, not just grep-noise. So the stronger lesson here is “fact re-derivation was the dominant hotspot in this profiling session,” not “clone elimination framing was wrong” in a broader roadmap sense. Right now the rest of the repo still says clone/ownership work is active, blocking, and strategically important. I’d keep the good merge_envs lesson, but avoid reprioritizing the whole lane in docs unless the surrounding roadmap and ratchets are updated to match. chatgpt-review-54264de8-5616-4c…

chatgpt-review-54264de8-5616-4c…

chatgpt-review-b056fa51-91f3-4c…

One non-blocking note: the new merge_envs → KF-2 story is good thesis material, but I’d phrase it more explicitly as future-state language. The roadmap still says KF-2 is not built and is blocked on KF-1, so “this is exactly the kind of bug KF-2 should eventually reject” is more accurate than present-tense wording that makes it sound like that contract already exists. chatgpt-review-54264de8-5616-4c…

No .dag files changed in this PR, so I don’t have compositional-modeling comments beyond that. Also, because this is docs-only, I’m treating the issues above as roadmap/docs alignment problems rather than direct invariant violations; the invariants themselves explicitly call out roadmap/docs drift as debt but not, by itself, a compiler invariant break. chatgpt-review-8976254b-5675-4b…

---

View conversation

[briansrls]

codex · gpt-5.4 · 28551d29

Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/4!)

INVARIANTS — Violations (2)

ROOT CAUSE ANALYSIS

• docs/perf/clone-elimination.md Missing upstream single-authority contract for InternTable and no declared std-level law for its merge operation -> merge_envs kept rebuilding from copied env state -> the docs now assign the symptom to KF-2; upstream fix is to encode the shared InternTable authority directly in the env/merge boundary so reconstruction is impossible, and only then, if needed, declare algebraic simplification in std/optimization.dag per MODELING.md M1/M9.
• docs/perf/clone-elimination.md The review guidance lacks a structural authority test -> it substitutes a surface-shape heuristic for explicit fact ownership -> reviewers will both over-flag lawful compositions and miss true single-authority violations; upstream fix is to ground the rule in No duplicate representations and Minimal information per interface by asking whether one input already carries the authoritative fact and, if so, reducing the API to that authority.

ROADMAP — Incomplete

• PERF status "the urgent perf crisis is resolved": This PR contains only documentation changes, so the roadmap advances PERF status without the supporting implementation or benchmark artifact in the same diff.
• PERF item "merge_envs re-derivation = 0 (Done)": The diff does not include the merge_envs code change that would justify marking this item complete.
• PERF item "Stages run on data-only files = 3 (Done — 82 of 143 files skip CX/ownership)": The same docs-only PR does not ship the compiler change needed to substantiate this done-state.
• KF-2 priority elevated from the merge_envs case study: The case study supports fixing boundary/fact-flow authority first; this diff does not demonstrate a KF-2 milestone advance or the missing std/optimization machinery.

The re-derivation diagnosis is directionally right, but the PR misclassifies the root cause as KF-2 and advances roadmap status beyond what this docs-only diff proves.

[briansrls]

Invariant violation: Calling merge_envs a KF-2/idempotency failure violates INVARIANTS.md Facts Flow Forward and Boundary sufficiency: the bug was downstream re-derivation after a single-authority fact existed upstream, not a missing optimizer pass.

[briansrls]

codex · gpt-5.4 · 28551d29

Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/4!)

INVARIANTS — Violations (2)

ROOT CAUSE ANALYSIS

• docs/perf/clone-elimination.md Missing upstream single-authority contract for InternTable and no declared std-level law for its merge operation -> merge_envs kept rebuilding from copied env state -> the docs now assign the symptom to KF-2; upstream fix is to encode the shared InternTable authority directly in the env/merge boundary so reconstruction is impossible, and only then, if needed, declare algebraic simplification in std/optimization.dag per MODELING.md M1/M9.
• docs/perf/clone-elimination.md The review guidance lacks a structural authority test -> it substitutes a surface-shape heuristic for explicit fact ownership -> reviewers will both over-flag lawful compositions and miss true single-authority violations; upstream fix is to ground the rule in No duplicate representations and Minimal information per interface by asking whether one input already carries the authoritative fact and, if so, reducing the API to that authority.

ROADMAP — Incomplete

• PERF status "the urgent perf crisis is resolved": This PR contains only documentation changes, so the roadmap advances PERF status without the supporting implementation or benchmark artifact in the same diff.
• PERF item "merge_envs re-derivation = 0 (Done)": The diff does not include the merge_envs code change that would justify marking this item complete.
• PERF item "Stages run on data-only files = 3 (Done — 82 of 143 files skip CX/ownership)": The same docs-only PR does not ship the compiler change needed to substantiate this done-state.
• KF-2 priority elevated from the merge_envs case study: The case study supports fixing boundary/fact-flow authority first; this diff does not demonstrate a KF-2 milestone advance or the missing std/optimization machinery.

The re-derivation diagnosis is directionally right, but the PR misclassifies the root cause as KF-2 and advances roadmap status beyond what this docs-only diff proves.

[briansrls]

codex · gpt-5.4 · 793f613d

Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/2!)

INVARIANTS — Violations (2)

ROOT CAUSE ANALYSIS

• THESIS.md Missing boundary contract that says TypeEnv already carries the authoritative InternTable and merge_envs must preserve it → re-derivation remains representable, the runtime pays O(n²), and the docs misdescribe the symptom as KF-2 → upstream fix: make that carried authority the primary model and treat any idempotent merge law from dsl/std/algebra.dag as secondary detection only, aligned with MODELING.md M1/M8/M9.

ROADMAP — Incomplete

• PERF status / current state: The section records the PR #422 speedup, Stages run on data-only files = 3, and merge_envs re-derivation = 0 as current state even though this PR contains no compiler changes and the text itself says the implementation is still pending merge.
• KF-2 priority elevation: This diff adds no std/optimization.dag, equivalence catalog, or compiler enforcement, so the milestone movement is rationale only, not code-backed progress in the same diff.

ROADMAP — Unclaimed

• Track 3: InternTable as identity consumer: The PERF narrative now depends on InternTable already being threaded through TypeEnv, but the Track 3 status row still says that threading is pending, so ROADMAP.md no longer has a single authoritative status for that dependency.

2 violations, 0 modeling issues found; the rewrite correctly shifts performance diagnosis from clone counts to fact re-derivation, but THESIS.md/ROADMAP.md still over-assign merge_envs-class bugs to KF-2 and ROADMAP.md overstates PR #422-backed progress without supporting code in this diff.

[briansrls]

Invariant violation: Calling merge_envs-class regressions "KF-2 we're committing against ourselves" violates INVARIANTS.md Facts Flow Forward and Boundary sufficiency by making a downstream equivalence checker sound like the primary authority instead of the upstream fact-preserving boundary.

[briansrls]

Invariant violation: Saying the merge_envs case's "structural fix is KF-2" violates Facts Flow Forward and Boundary sufficiency, because the structural fix is the upstream boundary contract that carries the authoritative InternTable, not a downstream cheaper-equivalent detector.

[briansrls]

codex · gpt-5.4 · 793f613d

Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/2!)

INVARIANTS — Violations (2)

ROOT CAUSE ANALYSIS

• THESIS.md Missing boundary contract that says TypeEnv already carries the authoritative InternTable and merge_envs must preserve it → re-derivation remains representable, the runtime pays O(n²), and the docs misdescribe the symptom as KF-2 → upstream fix: make that carried authority the primary model and treat any idempotent merge law from dsl/std/algebra.dag as secondary detection only, aligned with MODELING.md M1/M8/M9.

ROADMAP — Incomplete

• PERF status / current state: The section records the PR perf: skip data-only analysis, wire InternTable, fix merge_envs (~50% speedup) #422 speedup, Stages run on data-only files = 3, and merge_envs re-derivation = 0 as current state even though this PR contains no compiler changes and the text itself says the implementation is still pending merge.
• KF-2 priority elevation: This diff adds no std/optimization.dag, equivalence catalog, or compiler enforcement, so the milestone movement is rationale only, not code-backed progress in the same diff.

ROADMAP — Unclaimed

• Track 3: InternTable as identity consumer: The PERF narrative now depends on InternTable already being threaded through TypeEnv, but the Track 3 status row still says that threading is pending, so ROADMAP.md no longer has a single authoritative status for that dependency.

2 violations, 0 modeling issues found; the rewrite correctly shifts performance diagnosis from clone counts to fact re-derivation, but THESIS.md/ROADMAP.md still over-assign merge_envs-class bugs to KF-2 and ROADMAP.md overstates PR #422-backed progress without supporting code in this diff.

[briansrls]

ChatGPT Review

Grounded in the thesis, the headline is directionally right: the repo’s own doctrine says to fix the deepest missing fact, not the downstream compensation, and the performance invariant says the real fix is to stop losing facts rather than optimize the conservative fallback. So “clones are the symptom; fact re-derivation is the disease” is a good thesis-aligned diagnosis. chatgpt-review-54264de8-5616-4c…

I would leave three review comments.

1. Medium — the docs are promoting session measurements into source-of-truth claims before the checked-in authorities move.

In this PR, the rewritten roadmap/perf material presents new numbers and priorities as current repo truth, but the checked-in test authorities still say otherwise: PERF_RATCHET_SECONDS is still 120, and the tests still maintain clone-focused ownership ratchets plus a stage0 clone census as active metrics. The invariants are explicit that metadata should have a single producer and markdown should be derived from that authority, not become the authority itself. I would either phrase the new numbers as “profile results / pending PR #422 measurements” or update the actual ratchets and receipt-producing authorities in the same change.

1. Medium — the rewrite over-corrects from “clone count was not the dominant hotspot in this profile” to “clone-elision framing was wrong,” which is not where the current roadmap is.

The attached roadmap still treats LS-4 / Stream B clone elimination as active work: it keeps the three clone-elision layers, says borrow propagation is the bulk of the remaining unnecessary clones, and ties Layer 1 directly to Track 3 / Node.name deletion. In other words, the current project plan still says clone debt is real and strategically important; it just is not the only or even the dominant measured wall-clock bottleneck in every profile. I would keep the strong merge_envs lesson, but soften “that framing was wrong” to something like “that framing was wrong for this profiling session” or “clone count is not the priority perf metric for this bottleneck.”

1. Medium — the THESIS/KF-2 wording reads like a present-tense guarantee, but the roadmap still marks that capability as not built.

The new merge_envs case study is good motivation, and it does match the thesis’s algebraic-simplification direction. But the current roadmap still says KF-2 (“Reject suboptimal algorithms”) is not built and blocked on KF-1, while the thesis’s own correctness-dimensions table still says ownership is not yet carried on bindings and idempotence/effect algebra is declared but not consumed. So “this is the class of bug KF-2 should eventually reject” is accurate; “merge_envs-class bugs would be compile errors” reads too much like current-system truth. I would rephrase that section as future-state language rather than current doctrine.

No .dag files change in this PR, so I do not have compositional-modeling comments. Also, I would classify the issues above as docs/roadmap alignment concerns, not direct compiler invariant violations; the invariants explicitly say roadmap/docs drift and loose ratchets are not invariant violations by themselves.

---

View conversation