Repository navigation
PERF - #320
PERF#320
Conversation
Move structural eligibility analysis for fold acc unwrap optimization upstream: emitter no longer performs AST walks or heuristic condition chains. ownership.dag now produces FoldAccUnwrapProof, emitter only checks the proof + Rc rendering guard. - Define FoldAccUnwrapProof type in ownership.dag - Move fold_terminal_expr, fold_body_constructs_acc_struct, collect_acc_field_moves, fold_body_safe_field_moves to ownership.dag - Collect fold call nodes during walk_expr via UsageAccum (no extra pass) - Replace 5-condition block in emit_rust_fold_method_call with proof lookup - Convert fold tests from Rust substring assertions to behavioral proofs - Regenerate stage0 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff7c7e2e14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn build_fold_eligible_set(proof: OwnershipProof) -> Map<String, Bool> { | ||
| proof.fold_acc_unwrap |> fold(init: empty_map(), f: (acc, p) => | ||
| if p.eligible { map_insert(acc, p.acc_type_name, true) } else { acc } | ||
| ) |
There was a problem hiding this comment.
build_fold_eligible_set stores eligibility as acc_type_name -> true, which loses per-fold-call safety information; later emission reads only that type-level flag. In a function with two folds over the same accumulator type (one safe, one unsafe due to duplicate field moves), the safe fold marks the type as eligible and the unsafe fold will also get Rc::try_unwrap, causing Rust use-after-move compile failures. The eligibility index needs to distinguish fold call sites (or encode negative evidence), not just type names.
Useful? React with 👍 / 👎.
ChatGPT ReviewI reviewed the uploaded diff against the attached INVARIANTS.md and the ROADMAP.md changes. Overall: good direction, but I would not merge this as-is. The refactor is trying to do the right thing: move the structural part of fold-acc unwrap analysis out of emit_rust and leave the Rust-only rc_types guard in the Rust emitter. That split is aligned with the invariants’ “rendering is separate” / Theme 5 direction. The problem is that the new boundary is lossy and keyed at the wrong granularity, and that introduces a real codegen bug. Must-fix
In src/v2/05_emit_rust.dag, build_fold_eligible_set reduces the proof list to Map<acc_type_name, Bool> keyed only by type name (diff lines 195–199). That map is then installed once per function in emit_typed_item (diff lines 237–251), and emit_rust_fold_method_call decides unwrap by checking only emit_info.fold_eligible[acc_type_name] (diff lines 372–374). That means this breaks: dag If either fold proves eligible, the per-function map contains Accum -> true, and both call sites will unwrap. That is a real correctness bug, not just modeling debt. Fold unwrap eligibility is a call-site-local fact. The current representation turns it into a lossy summary, which is exactly the kind of boundary INVARIANTS.md warns against. Recommended fix: key by the fold method-call node (or stable node/span identity), not by acc_type_name. Best would be to attach the proof directly to the typed fold call or keep a per-node proof map.
Old emitter logic checked the terminal record literal against the real accumulator type (acc_type_node.name). The new ownership proof does this instead: derive acc_type_name from the terminal ExprRecordLit itself (diff lines 600–609), then call fold_body_constructs_acc_struct(..., acc_type_name) (line 615), which re-walks to the same terminal and checks terminal.name == acc_type_name (diff lines 537–549). So for any terminal ExprRecordLit, cond_body is effectively self-justifying. It no longer means “the body constructs the fold’s accumulator type”; it means “the body ends in a record literal, and I named the type after that record literal.” That is a semantic regression from the old check, and it is also a compositional-modeling issue: you already have the actual fold accumulator type in the fold emission path, but the new proof re-derives a weaker proxy from downstream shape. Recommended fix: use the actual fold accumulator type for the current fold call, not the terminal record literal name. Even better, carry a node/ref, not a bare string.
The two fold regression tests in src/v2/tests/src/pipeline.rs used to assert actual emitted Rust (Rc::try_unwrap(acc) present/absent, moved field vs cloned field). In this PR they now only assert ownership proofs (diff lines 1173–1244). That means the new bug above can pass tests even if codegen is wrong. Recommended fix: keep the ownership-proof assertions, but restore at least one end-to-end emission assertion. Also add a regression with two folds of the same accumulator type in one function, one eligible and one not. Invariant / modeling assessment Moving the Rust-only cond_rc check to emit_rust_fold_method_call is good. That part belongs in the Rust renderer. Still violating the boundary rules The new fold_eligible_index / fold_eligible boundary is too lossy: it drops call-site identity, it drops the actual accumulator-type witness, it drops the underlying structural reasons and keeps only eligible, it keys by naked strings. That lands squarely in the INVARIANTS.md problem family: Heuristics indicate lost structure No duplicate / lossy representations Boundary fact tables must be exact derivations I would not model this as Map<FuncName, Map<AccTypeName, Bool>>. A better shape would be something like a per-fold structural proof: fold call site id/span, accumulator binding, actual accumulator type, “terminal constructs fresh accumulator of that type,” “field moves are unique.” Then emit_rust can combine that with rc_types and any Rust-only repr/wrapping facts. Slight target-leakage concern FoldAccUnwrapProof and fold_eligible_* are renderer-facing names/shapes living in the ownership/shared-emit boundary. The underlying proof can be target-agnostic, but the current API is Rust-shaped. I would prefer a target-neutral proof object in ownership.dag, and let emit_rust decide whether that proof justifies Rc::try_unwrap. Smaller concerns Old emitter-side collect_acc_field_moves excluded borrow-only uses under rt_ref_map_functions() (removed diff lines 294–317). The new ownership version explicitly says it “counts every access as a move” (diff lines 551–568). That is safe, but it will produce more false negatives: folds that were previously unwrap-eligible may no longer be. That is acceptable as a temporary conservative step, but it should be called out as a behavior change.
Both acc_type_name in the proof and the fold_eligible map are string-keyed. Even aside from the main bug, this is a fragile identifier choice for a proof boundary. A stable node identity/span/ref would be much safer. ROADMAP alignment The added ROADMAP.md content about the bootstrap dependency chain and BP-1 build-pipeline modeling looks internally coherent and broadly aligned with the repo’s “make derivation chains explicit” direction. I do not see a roadmap conflict there. I do think it is orthogonal to this PR’s main change. If you want cleaner review boundaries, I’d split the roadmap/doc addition out from the fold-unwrap refactor. Bottom line Needs changes before merge. The big blocker is this: the old logic made an unwrap decision for the current fold call, the new logic makes it for any fold of this accumulator type in this function. That is the wrong abstraction boundary and can miscompile code. The minimum fix set I’d want: make eligibility per fold call site, not per function/type; use the actual accumulator type, not the terminal record literal name; restore end-to-end emitter assertions and add the two-fold same-type regression test. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 4, MODELING: 2+/3-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (4)
MODELING — Strengths
src/v2/ownership.dagMoving fold-unwrap analysis out of emit_rust and into ownership.dag is directionally better because it tries to ground the optimization in ownership structure before rendering.src/v2/05_emit_rust.dagLeaving the final Rc guard in the Rust backend while moving structural eligibility earlier is better layering than the previous all-in-emitter heuristic.
MODELING — Improvements
src/v2/ownership.dagFoldAccUnwrapProof should be keyed by structural fold-site identity and authoritative accumulator type, not acc_param_name or acc_type_name strings re-derived from syntax.src/v2/05_emit_rust.dagOwnershipBuildResult is still an emitter-local side table built from Map<String, Map<String, Bool>>; the more compositional model is to carry exact fold-site proofs on the typed boundary and let emit consume them directly.src/v2/04_emit_info.dagAdding fold_eligible_index and fold_eligible to EmitGraphInfo duplicates ownership facts in an emit bundle and uses string-keyed identity proxies, which is less faithful than carrying the proof itself or a structural Node reference.
ROADMAP — Incomplete
- Bootstrap Health Rules: stage0 generated .rs files use -merge in .gitattributes: The diff adds this as a current rule but lands no .gitattributes change or generated pipeline authority, so the claimed enforcement is unsupported by code in the PR.
ROADMAP — Unclaimed
- CG-1 Sharing and ownership: The PR partially advances the move from emitter heuristics toward ownership-derived fold unwrap facts, but ROADMAP does not record that partial progress in the CG ownership work items.
Directionally this moves a Rust heuristic upstream, but the new fold-unwrap proof is still emitter-local, lossy, and name-proxy-based, so it is not yet an invariant-clean root-cause fix.
| ownership_index: Map<String, Map<String, Bool>> | ||
| fold_eligible_index: Map<String, Map<String, Bool>> | ||
| } | ||
|
|
There was a problem hiding this comment.
Invariant violation: build_fold_eligible_set keys eligibility only by acc_type_name, so two fold sites of the same accumulator type in one function are collapsed into one boolean; that is a lossy string-proxy fact table and violates Explicit boundary contracts and M4 direction.
| @@ -371,15 +384,17 @@ fn build_ownership_index(modules: List<TypedModule>) -> Map<String, Map<String, | |||
|
|
|||
| fn emit_rust(typed: ResolvedGraph) -> EmitResult { | |||
| let base_info = build_emit_graph_info(modules: typed.modules) | |||
There was a problem hiding this comment.
Invariant violation: emit_rust now recomputes fold unwrap eligibility with analyze_ownership inside the Rust backend instead of consuming it from an upstream boundary, so emission is still making semantic decisions in violation of Emission is translation, not decision-making.
| _ => false | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Invariant violation: collect_acc_field_moves is explicitly conservative and counts every acc.field access as a move, but emit consumes the resulting eligible bit as authority, so the new fact table is lossy rather than an exact derivation and violates Explicit boundary contracts.
| _ => "" | ||
| } | ||
| // Derive acc type name from the lambda body's terminal ExprRecordLit. | ||
| let acc_type_name = match fold_lambda_node.expr_data { |
There was a problem hiding this comment.
Invariant violation: analyze_single_fold derives acc_type_name from the terminal ExprRecordLit name itself, so the proof re-derives identity from Node.name instead of checking against the authoritative accumulator type, violating Boundary sufficiency and M4.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 2+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (2)
MODELING — Strengths
src/v2/ownership.dagMoving fold-unwrap eligibility intoownership.dagis the right layer move because it turns a Rust leaf heuristic into an explicit ownership fact.src/v2/05_emit_rust.dagThreading fold eligibility throughEmitGraphInfois more compositional than re-inspecting each lambda body at the emission site.
MODELING — Improvements
src/v2/ownership.dagFold eligibility would compose more faithfully if the proof were keyed by structural fold-site identity and carried through the existing ownership walk, rather than summarized in a secondary name-based post-pass.src/v2/05_emit_rust.dagOwnershipBuildResultstill computes a shared semantic fact inside the Rust backend, so the model is one layer too low; the proof should arrive from an upstream boundary that all backends can consume.
ROADMAP — Incomplete
- Bootstrap Health Rules / stage0 merge handling: ROADMAP now says generated stage0
.rsfiles use-mergein.gitattributes, but this diff contains no.gitattributeschange or generator that would make that claim true.
ROADMAP — Unclaimed
- PERF tokenizer regression coverage: The new tokenizer timing tests advance the PERF track with a concrete performance ratchet, but the PERF section was not updated to record that new coverage.
The PR moves fold unwrap toward an explicit proof boundary, but it still adds a parallel ownership walk, regresses tests to internal-proof assertions, and overstates roadmap progress on stage0 merge handling.
| let result = walk_expr(accum: initial, texpr: body, in_tail: true) | ||
| // Build decisions from the collected usage data. | ||
| let binding_list = result.bindings |> map_values | ||
| let decisions = binding_list |> map(usage => make_decision(usage: usage)) |
There was a problem hiding this comment.
Invariant violation: analyze_ownership now collects fold nodes and then re-walks them with analyze_single_fold, adding a new parallel ExprData walk instead of extending the existing ownership walk, which violates the v2 "no new ExprData walks without deleting an existing one" rule and No parallel implementations.
| // Rc::try_unwrap should be emitted to convert Rc<Accum> to owned Accum | ||
| // Behavioral: ownership proof should mark fold accumulator as eligible | ||
| let proof = result | ||
| .ownership |
There was a problem hiding this comment.
Invariant violation: These fold-unwrap regressions now assert the internal fold_acc_unwrap proof bit rather than emitted/output behavior, violating Testing Invariants: Behavioral only.
ChatGPT ReviewReviewed from the provided diff. My take: I would not merge this as-is. I see one correctness blocker, plus a couple of important modeling/test concerns. 1. Blocker: fold unwrap eligibility is being collapsed from “this fold call is safe” to “this accumulator type is safe somewhere in this function.” In That is not just a modeling nit. It can mis-emit 2. The direction is good, but the new proof is not yet structurally exact enough. Moving the unwrap proof out of the Rust emitter and into But the current proof drops important distinctions:
I’d prefer the proof to carry the resolved accumulator type node (or stable type id) from the init arg, then compare the terminal body expression against that authoritative type. Right now it is too easy for name/alias mismatches to become either false negatives or accidental broadening. 3. The tests got weaker exactly where this PR changes semantics.
I’d keep the proof-level assertions, but also keep at least one end-to-end emitted-output assertion, or better, a bootstrap/cargo-check fixture that proves the optimized fold still compiles. 4. Roadmap/doc note: mostly aligned, but one statement reads ahead of the code. The new One caution: the added text says generated stage0 5. Small concern: the new tokenizer perf tests may be flaky in CI. The two new tests in So the merge decision for me is:
One positive note: the stage0 Rust updates look mechanically consistent with the |
…(1) tokenizer substring - emit_data_def wraps non-Copy data defs in thread_local! caching (maps, records, JSON) - Add chars_to_string runtime function for O(end-start) substring from pre-computed codepoints - Add append runtime function (list push through Rc) - Remove 5 cp lines from regenerate-stage0.sh: v2_compiler_infer_method.rs, std_types.rs, extdeps_languages_rust_emit.rs, v2_compiler_tokenize.rs, extdeps_languages_dag_syntax.rs - Fix O(n²) tokenizer regression: source_substring now uses chars_to_string instead of substring - Add tokenizer performance regression tests (linear scaling, non-ASCII parity) - Update ROADMAP: 4 hand-maintained files remain (main.rs, v2_rt.rs, compiler_tests.rs, v2_coercion.rs) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # src/v2/stage0/src/std_types.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 1+/2-, ROADMAP: 2✓/2!)
INVARIANTS — Violations (3)
MODELING — Strengths
dsl/extdeps/languages/rust/emit.dagAddingchars_to_stringthroughrt_function_registryand its derived lookup tables keeps the Rust runtime bridge data-driven and correctly layered at L3.
MODELING — Improvements
src/v2/ownership.dagThe fold-unwrap model would be more compositional if it exposed exact ownership witnesses over structural nodes and left Rust-specific unwrap eligibility to the renderer.src/v2/04_emit_info.dagfold_eligible_indexandfold_eligibleare lossy boolean maps over names; a faithful boundary would carry per-fold structural proofs rather than name-keyed eligibility bits.
ROADMAP — Verified
- Bootstrap Status hand-maintained files:
scripts/regenerate-stage0.shnow copies back onlymain.rs,v2_rt.rs,compiler_tests.rs, andv2_coercion.rs, matching the revised claim. - Previously eliminated PR #316 files: The diff removes those five copy-back lines and regenerates their stage0 implementations with cached outputs, so the "previously eliminated" claim is supported by code.
ROADMAP — Incomplete
- Bootstrap Dependency Chain hand-maintained files: The new dependency-chain note still says there are 9 copied-back hand-maintained files, but this diff's regeneration script keeps only 4.
- Bootstrap Health Rules
.gitattributes-mergerule: ROADMAP now claims generated stage0 Rust already uses-merge, but the diff contains no.gitattributeschange or generator support for that rule.
ROADMAP — Unclaimed
- PERF-4 test suite wall-clock ratchet: The new tokenizer/parser timing regressions materially advance PERF wall-clock coverage, but ROADMAP does not record that progress in the PERF track.
The perf work fixes a real tokenizer bottleneck, but it also adds new backend-local semantic recovery in Rust emit and over-claims part of the bootstrap/merge roadmap state.
| @@ -96,10 +99,19 @@ type OwnershipDecision | |||
| | SharedError { binding: String, consumer_count: Int, sites: List<String> } | |||
There was a problem hiding this comment.
Invariant violation: FoldAccUnwrapProof/fold_acc_unwrap turns a Rust Rc::try_unwrap optimization into shared ownership authority instead of carrying target-agnostic ownership facts, violating Explicit boundary contracts and Emission is translation, not decision-making.
| if needs_rc { concat("Rc::new(", raw, ")") } else { raw } | ||
| } | ||
|
|
||
| // Peephole: fuse skip(n) followed by first into .get(n) for O(1) indexed access. |
There was a problem hiding this comment.
Invariant violation: emit_rust_first_method_call branches on the method name string "skip" to choose a backend peephole, violating No case enumeration for open sets and Emission is translation, not decision-making.
|
|
||
| fn emit_typed_tco_reassign(args: List<Node>, params: List<Node>, registry: Map<String, ItemInfo>, scope: InferScope, depth: Int, rc_types: Map<String, Bool>, emit_info: EmitGraphInfo) -> String { | ||
| let ordered_args = args |> map(a => emit_typed_expr(texpr: arg_value(n: a), registry: registry, scope: scope, depth: depth, rc_types: rc_types, emit_info: emit_info, fuel: 1024)) | ||
| let si = scope.type_env.source_index |
There was a problem hiding this comment.
Invariant violation: emit_typed_tco_reassign adds expr_references_var and recomputes tco_movable inside Rust emit, creating a new downstream Expr walk and ownership proof in violation of No parallel implementations and Root-Cause Depth.
1 participant
Summary
ownership.dag— target-agnostic proof encoding structural eligibility for fold accumulator move optimizationfold_terminal_expr,fold_body_constructs_acc_struct,collect_acc_field_moves,fold_body_safe_field_moves) from05_emit_rust.dag→ownership.dagemit_rust_fold_method_callwith upstream proof lookup + Rc rendering guardwalk_exprtraversal viaUsageAccum.fold_call_nodes(no extra O(n) pass)Invariant theme: Graph proves, emit renders.
Test plan
cargo test -p v2-compiler-tests— 284 tests passcargo test --workspace --exclude v2-compiler-tests— 19 tests passcargo clippy --all-targets -- -D warnings— cleancargo test -p v2-compiler-tests v2_strict_compile_diagnostic_count -- --ignored— ratchet at 0scripts/regenerate-stage0.sh— 0 diagnostics🤖 Generated with Claude Code