Repository navigation
Conversation
Moves docs/perf-borrow-design.md → docs/perf/clone-elimination.md and rewrites it as a concrete investigation plan with explicit review questions. This PR exists to get thorough review of the methodology before the perf session chases specific clones without profile data. Corrects the previous draft on several points raised in review: - Accurate baseline: 59 stage0 files, 54,568 lines, 21,211 .clone() sites (was: stale 13,694) - Correct cost model: Rc<T>.clone() is atomic refcount++, String::clone() is heap alloc. Not equivalent. - Credits landed work: read_only_params_index (05_emit_rust.dag:471) already implements selective borrowing. render_node_type already emits borrowed params. This doc is about the REMAINING work in excluded categories, not a missing Phase 1. - Narrower movable set: ownership.dag:402 only marks fan_out == 1 as movable. General last-use analysis for fan_out > 1 does NOT exist — it's genuine work to build. - Field access has special cases: tuple/option/enum/anonymous-record flattening + owned_bindings after Rc::try_unwrap. A blanket &n.field rewrite is not correct. - Stream D (PR #419) merged but only -3 CX violations (expected -137). Flagged as separate investigation. - Rust-local scope: this is NOT LS-4 design. LS-4 spans borrow semantics per target (SharingStrategy in languages.dag:174). Python/Go use GC, so Rc clone elimination doesn't apply. Structure of the doc: - Verified current state (numbers, per-file breakdown) - What's already landed (read_only_params_index, movable, last-use) - What's NOT borrowed (4 exclusion categories with file refs) - Corrected clone cost model - Honest priority list (M2 Node.name is #1 — String heap alloc) - Investigation plan (5 steps, each producing a decision) - Known unknowns (single-thread, CI variance, Stream D CX gap, test harness overhead) - Non-goals (no emitter changes without profile, no ratchet bumps) - Review questions for reviewers to answer Goal: ship the investigation plan after review. The perf session should profile first, target the biggest heap allocation source (probably String clones), and validate on single functions before scaling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewI’d request changes on this PR. This is a doc-only change: chatgpt-review-6a1ae2be-ce4d-46… chatgpt-review-7954939d-3fa3-45… chatgpt-review-340e7f84-1093-4f…
The new doc says chatgpt-review-705949f4-de28-4e…
The new doc says Stream D’s CX impact was chatgpt-review-340e7f84-1093-4f…
Saying “this investigation only changes the Rust emitter” is fine. Saying “Python/Go use GC, so Rc-style clone elimination doesn’t apply to them” goes too far. ROADMAP’s LS-4 is explicitly a cross-target language-spec problem with Rust/Go/Python-specific sharing/borrowing semantics, and THESIS says target behavior belongs in specs, not in separate target-specific mechanisms. INVARIANTS also calls out that the sharing model being Rust-only is still an unresolved gap, not a settled design target. I would rephrase this as “implementation focus: Rust hot paths only,” not “architecture: clone elimination is Rust-only.” chatgpt-review-4e475471-ab09-4b… chatgpt-review-7954939d-3fa3-45… chatgpt-review-6a1ae2be-ce4d-46…
“Profile first” is a good prioritization rule. But the doc currently risks sounding like profiler evidence is a prerequisite for already-known root-cause work. INVARIANTS’ “Facts Flow Forward” and Root-Cause Depth rules say the real fix is to stop losing facts upstream, not to optimize compensating behavior downstream. ROADMAP already names concrete structural work here: Node.name deletion, stable binding identity, output provenance, and LS-4. I’d change the wording to something like: “profile first to prioritize among known structural fixes,” which keeps the investigation aligned with the repo’s causal-engine thesis. chatgpt-review-6a1ae2be-ce4d-46… chatgpt-review-7954939d-3fa3-45… So my merge recommendation is:
The |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 6861a077
Review (INVARIANTS: 3, MODELING: 0+/0-, ROADMAP: 1✓/2!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
docs/perf/clone-elimination.mdThe upstream gap is that call-mode and sharing facts are still lost at the function-call boundary, so the doc reaches for downstream Rust-specific wrappers; model the call convention once in the graph/LanguageSpec and derive any needed adapter from that single authority instead of proposing ad-hoc borrowed/owned sibling functions (M1, M8, M9; "Emission is translation, not decision-making").docs/perf/clone-elimination.mdBecause the doc treats performance_ratchet as the only measurement authority, it compensates for missing stage data by inventing a second profiler in compile_sources; reuse or extract the existing profile_full_pipeline path as the single authority for stage timing and have the ratchet/reporting consume that shared helper instead (M2, M7).docs/perf/clone-elimination.mdThe file moved one directory deeper but its authority links were copied unchanged, so readers now follow a stale path instead of the real source of truth; rebase them to ../../ROADMAP.md and ../../THESIS.md so this doc continues to point at the canonical authorities rather than an implicit duplicate path (M2/M7 single authority).
ROADMAP — Verified
- PERF design reference: ROADMAP.md now points to docs/perf/clone-elimination.md, and that file is added by the same diff.
ROADMAP — Incomplete
- PERF status prose: The surrounding PERF section still says parser state threading is an outstanding clone bucket even though the new design doc says Stream D already removed ParserState from the parser path.
- PERF root-cause metric: The roadmap still presents 23,733 Rc::clone() as the measured root cause while the new design doc says the previous numbers were stale and corrects current state to 21,211 clone sites.
0 modeling issues found because no .dag types changed, but the docs still introduce three single-authority/thesis regressions that should be fixed before merge.
|
|
||
| 3. **Reduce callable-set exclusions** — if a function is in the | ||
| callable set AND called directly, its direct call sites miss | ||
| borrow benefits. Dual-emission (borrowed for direct, owned |
There was a problem hiding this comment.
Invariant violation: The medium-impact plan elevates borrowed-version-plus-owned-wrapper variants for callable and TCO functions, violating INVARIANTS.md "No parallel implementations" and THESIS.md concept unification by splitting one semantic function into multiple emitted authorities.
| emit)? If one stage is >50% of time, that's the bottleneck | ||
| regardless of clone count. | ||
|
|
||
| ### Step 2: Add per-stage instrumentation if missing |
There was a problem hiding this comment.
Invariant violation: Step 2 proposes adding per-stage timing inside compile_sources even though src/v2/compiler_tests_rust.dag:934 already provides a full-pipeline stage breakdown, violating INVARIANTS.md "No duplicate representations" and "No parallel implementations."
| @@ -0,0 +1,459 @@ | |||
| # PERF: Clone Elimination — Current State and Remaining Work | |||
|
|
|||
| Part of: [ROADMAP.md §PERF](../ROADMAP.md#perf-eliminate-unnecessary-work) | | |||
There was a problem hiding this comment.
Invariant violation: The moved file still links to ../ROADMAP.md and ../THESIS.md, which now resolve to non-existent docs/ copies and violate INVARIANTS.md "Single-authority metadata" by disconnecting the investigation doc from the canonical thesis and roadmap.
|
Review (INVARIANTS: 3, MODELING: 0+/0-, ROADMAP: 1✓/2!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
ROADMAP — Verified
ROADMAP — Incomplete
0 modeling issues found because no .dag types changed, but the docs still introduce three single-authority/thesis regressions that should be fixed before merge. |
|
Superseded by #423. The investigation plan in this PR pre-dates the dark-emu profiling findings, which invalidated the clone-elimination framing. The actual perf win came from a 6-line fix to merge_envs (fact re-derivation), not from clone reduction. #423 captures the corrected framing with the real case study. Closing to consolidate review on a single PR. |
Fixes from Codex reviews on #420 and #421: - Correct stale 23,733 clone count in ROADMAP LS-4 section to ~13,724 (verified post-dark-emu) with cost model context - Remove references to transition-relations.md (that PR was closed — the concept invented a parallel umbrella authority) - Add "Cross-pass composition" section explaining the CORRECT framing: SubValueRelation + TerminationProof + read_only_params already exist as single authorities. The gap is threading SubValueRelation across the call-site boundary, not inventing a replacement concept. M1 Step 3 is that threading. - Reconcile Stream D blocker: it's output provenance on function signatures (the existing plan), not "transition relations" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Purpose
Get thorough review of the perf investigation methodology BEFORE the perf session chases specific clones without profile data. This is a design review PR, not an implementation PR.
The file moves from `docs/perf-borrow-design.md` → `docs/perf/clone-elimination.md` so the entire doc appears in the diff (reviewers can see everything, not just incremental changes).
What changed from the previous draft
The previous draft had stale numbers and an incorrect cost model. This rewrite:
Review questions (please address)
🤖 Generated with Claude Code