Repository navigation
PERF: eliminate O(N²) HashMap cloning in complexity analysis - #316
Conversation
The complexity analyzer threads Rc<CostInternTable> through every cost_of_expr call. Each cache_summary() cloned the entire HashMap via Rc::make_mut() (refcount always >1 due to table sharing), creating O(N²) allocations for N functions. Replace with a thread_local RefCell<HashMap> cache. cache_summary and lookup_summary now redirect to the thread-local, making the threaded Rc<CostInternTable> a no-op passthrough. The generated code structure is unchanged — only the 3 cache access points are modified. This is a bootstrap bridge patch (stage0 only). The corresponding complexity.dag change is deferred to CX track. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 2e2805c.
![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: 2e2805cbae
ℹ️ 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".
| pub fn cache_summary(table: Rc<CostInternTable>, func_name: String, summary: Rc<ComplexitySummary>) -> Rc<CostInternTable> { | ||
| Rc::new(CostInternTable { | ||
| summaries: v2_rt::rc_map_insert(table.summaries.clone(), func_name, summary), | ||
| }) | ||
| SUMMARY_CACHE.with(|cache| { | ||
| cache.borrow_mut().insert(func_name, summary); | ||
| }); | ||
| table // PERF: return same table — real cache is thread-local |
There was a problem hiding this comment.
This edits a generated stage0 file directly, but the source generator (src/v2/complexity.dag) still defines cache_summary/lookup_summary with map_insert/map_get. CI runs scripts/check-stage0-freshness.sh via the Stage0 Freshness workflow step, and that gate is designed to fail when committed stage0 differs from regenerated output, so this change will be overwritten on regeneration and will block CI until the .dag source (or regeneration pipeline) is updated too.
Useful? React with 👍 / 👎.
| }) | ||
| }); | ||
| let summaries_map = result.table.clone().summaries.clone(); | ||
| let summaries_map = SUMMARY_CACHE.with(|cache| Rc::new(cache.borrow().clone())); |
There was a problem hiding this comment.
After this change, summaries are exported from SUMMARY_CACHE instead of result.table.summaries, but ComplexityReport still carries intern_table: result.table. Because cache_summary now returns the input table unchanged (initialized from empty_intern_table()), callers that read report.intern_table.summaries will see an empty map even when report.function_summaries is populated, creating a self-inconsistent report payload.
Useful? React with 👍 / 👎.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (2)
ROADMAP — Incomplete
- Bootstrap D: The PR hand-edits generated stage0 Rust, so it does not satisfy the roadmap gate requiring regeneration-based freshness with zero manual stage0 patches.
This is a real performance hotspot, but the fix lands as a stage0-only hidden-state workaround rather than an upstream source-level authority change.
| // The generated code threads Rc<CostInternTable> through every call, | ||
| // but cache_summary/lookup_summary redirect here instead of cloning | ||
| // the inner HashMap on every insert via Rc::make_mut(). | ||
| thread_local! { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| SUMMARY_CACHE.with(|cache| { | ||
| cache.borrow_mut().insert(func_name, summary); | ||
| }); | ||
| table // PERF: return same table — real cache is thread-local |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ChatGPT Review (extended thinking)Request changes. I only see one changed file in this PR: src/v2/stage0/src/v2_compiler_complexity.rs. There are no .dag changes, and ROADMAP.md is not modified. So the .dag modeling and roadmap-update checks are mostly N/A on the surface, but that is also the main architectural issue here: this patch changes the behavior of generated stage0 code without changing the source authority it is supposed to be derived from. Blocking: this breaks the CostInternTable contract rather than optimizing it. The source model for complexity analysis is explicit: CostInternTable is the cache, it is threaded through analysis calls, each call may return an updated table, build_complexity_report reads result.table.summaries, and ComplexityReport stores intern_table: result.table. complexity complexity complexity This PR changes only the stage0 Rust to do something materially different: introduce thread_local! SUMMARY_CACHE make cache_summary(...) mutate TLS and return the same table make lookup_summary(_table, ...) ignore the table parameter entirely rebuild summaries_map from TLS at the end of build_complexity_report That means CostInternTable is no longer the authority; it becomes a dummy token while the real state lives off to the side. That violates the repo’s “boundary sufficiency” / “explicit boundary contracts” rules: downstream state is no longer represented in the boundary type, and the same logical fact now exists in two places. INVARIANTS INVARIANTS There is also a likely concrete inconsistency here: the source ComplexityReport still includes intern_table: result.table, but this patch only changes how function_summaries is populated. So function_summaries and intern_table can now disagree by construction. Even if nothing reads intern_table today, that is exactly the kind of silent divergence these invariants are trying to prevent. complexity complexity Blocking: stage0/source authority drift. The invariants and roadmap are very explicit that stage0 is generated from .dag source, is not supposed to be hand-edited, and the intended bootstrap loop is regenerate → diff → empty. The current bootstrap goal is to make stage0 regeneration authoritative again, not to add more stage0-only logic. INVARIANTS ROADMAP But this PR changes only src/v2/stage0/src/v2_compiler_complexity.rs. It does not change src/v2/complexity.dag, which still defines the old persistent-table semantics. So after this PR: .dag says the cache is CostInternTable generated stage0 says the cache is thread-local mutable state That is a direct dual-authority problem. The next regeneration either drops this fix or forces another manual patch. I would not merge a semantic change like this unless it lands in the .dag source and stage0 is regenerated from that single source of truth. INVARIANTS ROADMAP Major: hidden mutable ambient state introduces reentrancy and lifecycle hazards. clear_summary_cache() at the start of build_complexity_report() makes the function non-reentrant on a thread. A nested or re-entrant call on the same thread will wipe the outer call’s cache. Even without recursion, TLS is now part of the function’s observable behavior, so this core analysis code is no longer a pure “inputs → outputs” computation. That runs against the repo’s engineering guidance to prefer returned values over shared mutation and keep core logic pure. Pasted markdown There is also a smaller lifetime concern: the cache is cleared on entry, not on exit, so the whole map stays resident in TLS until the next run or thread teardown. That is not necessarily fatal, but it is another consequence of moving state out of the returned value and into ambient storage. The performance diagnosis is right; the fix location is wrong. I agree with the PR’s root problem: repeated whole-HashMap cloning in a hot path is exactly the kind of accidental quadratic behavior the repo treats as a correctness bug. The invariants also say the fix should be structural, not a downstream compensation hack. INVARIANTS There is even already tracked context pointing at this exact area: the complexity guard for large modules was documented as a temporary workaround, and the stated fix was to move the intern table to arena allocation or RefCell instead of deep Rc cloning. 195 So the direction is sensible, but the RefCell needs to live in the authoritative state, not in thread-local side state. Better options: make CostInternTable own interior-mutability directly, so the table parameter remains the authority or make build_complexity_report use explicit build-local mutable state that is still passed structurally and in either case, land it in src/v2/complexity.dag and regenerate stage0 .dag compositional modeling quality No .dag files changed, so there is no positive compositional-modeling work to review in this PR. But from a modeling standpoint, this change is negative: it replaces an explicit modeled boundary (CostInternTable) with hidden runtime state. In this codebase, that is the opposite of compositional modeling. Roadmap alignment ROADMAP.md is not modified, so there is no roadmap diff to validate. Directionally, though, this is misaligned with the current bootstrap work: the roadmap snippets I found emphasize that manual stage0 edits are the productivity failure to eliminate, and that the milestone is making regeneration authoritative again. This patch moves further away from that. ROADMAP What I would ask for before merge Move the fix into src/v2/complexity.dag and regenerate stage0. Preserve CostInternTable as the single cache authority. Add a regression test that proves report.function_summaries and report.intern_table stay consistent. Add a test that repeated build_complexity_report() calls on the same thread do not leak or interfere. If a temporary stage0-only bootstrap patch is unavoidable, it should still preserve the table contract and come with an explicit removal ratchet. This one does not. So: real perf bug, not mergeable as implemented. The patch removes the O(N²) symptom by making the cache invisible to the type/API boundary, and that collides with the repo’s invariants hard. |
…y_unwrap Model SharingStrategy.fold_accumulator_shared in languages.dag, then emit Rc::try_unwrap(acc) at fold iteration start so field accesses move instead of clone. A static safety check (collect_acc_field_moves) rejects the optimization when any accumulator field is moved more than once, preventing use-after-move errors. Stage0 regenerated via two-pass bootstrap. 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: 0✓/0!)
INVARIANTS — Violations (3)
MODELING — Strengths
src/v2/languages.dagKeeping SharingStrategy inside LanguageSpec places target-specific sharing syntax at the correct language layer instead of in shared compiler IR.
MODELING — Improvements
src/v2/languages.dagfold_accumulator_shared: Bool is not compositional; the model should express fold/update ownership through the same sharing/rendering algebra as wrap_template, not a target-global boolean switch.src/v2/04_emit_info.dagEmitGraphInfo is the right boundary for consumed facts, but owned_bindings should be keyed by structural binding identity rather than String so shadowing cannot corrupt ownership decisions.
ROADMAP — Unclaimed
- PERF track: The PR advances performance by removing clone-heavy paths in stage0 complexity and fold emission, but ROADMAP.md records no corresponding PERF progress, metric change, or ratchet update.
The prior stage0 cache issue is fixed, but the new fold optimization still breaks invariants by adding emitter-local name-keyed ownership authority and a forbidden runtime clone fallback.
| // Bindings known to be owned (not behind Rc). Field accesses on these | ||
| // bindings use move semantics instead of .clone(). Set for fold | ||
| // accumulators after Rc::try_unwrap when fold_accumulator_shared is false. | ||
| owned_bindings: Map<String, Bool> |
There was a problem hiding this comment.
Invariant violation: owned_bindings: Map<String, Bool> makes ownership depend on binding names at the emit boundary, violating Boundary sufficiency and No duplicate representations; the fact must flow as structural binding identity from ownership analysis.
| let needs_unwrap = acc_name != "" && emit_map_has(m: emit_info.owned_bindings, key: acc_name) | ||
| if needs_unwrap { | ||
| let acc_ident = emit_ident(name: acc_name, target: Rust) | ||
| concat("|", params_str, "| { let ", acc_ident, " = Rc::try_unwrap(", acc_ident, ").unwrap_or_else(|rc| (*rc).clone()); ", body_str, " }") |
There was a problem hiding this comment.
Invariant violation: Emitting Rc::try_unwrap(...).unwrap_or_else(|rc| (*rc).clone()) is the exact runtime fallback forbidden by No fallbacks that fabricate, because emit is compensating for missing upstream single-consumer proof instead of consuming it structurally.
| match lambda_node.expr_data { | ||
| ExprLambda { semantics: _ } => | ||
| let body = lambda_body(texpr: lambda_node) | ||
| let terminal = fold_terminal_expr(body: body) |
There was a problem hiding this comment.
Invariant violation: terminal.name == acc_type_name uses Node.name as semantic authority for the fold optimization, violating Boundary sufficiency and Emission is translation, not decision-making; this construction fact should arrive structurally from infer/reconcile.
ChatGPT Review (extended thinking)Request changes. The clone-reduction idea is good, but this PR is doing two very different things at once: a stage0 perf cleanup, and a new Rust-emitter ownership optimization for fold accumulators. The first looks directionally right. The second is where I think the risk is, and I would not merge it as-is.
Highest severity: this reintroduces the exact fallback pattern that INVARIANTS.md calls out as invalid. The doc explicitly says ownership should not compile to Rc::try_unwrap(x).unwrap_or_else(|rc| (*rc).clone()); either the compiler proves single ownership and emits a move, or it surfaces that the proof is missing. This PR adds that exact pattern in the new fold-emission path and also in several stage0 folds. That conflicts directly with both “No fallbacks that fabricate” and the repo’s “performance is correctness / facts flow forward” rule. Pasted markdown INVARIANTS The emitter is making a new ownership decision by heuristic, instead of consuming an upstream fact. INVARIANTS.md is very explicit that emit should be pure translation, not semantic decision-making, and that new fact tables must be exact, non-lossy, and consumed as authority. Here the new owned_bindings map is synthesized inside emit from local AST shape checks (fold_body_constructs_acc_struct, collect_acc_field_moves, fold_body_safe_field_moves) and then used to change clone-vs-move behavior. That is exactly the “fact was lost, downstream guessed” pattern the invariants warn against. INVARIANTS 211 This also looks like a duplicate ownership authority. EmitGraphInfo already had movable, sourced from ownership analysis. This PR adds owned_bindings, also about move semantics, but computed ad hoc in the emitter and keyed only by name. That is a second authority for related semantics, which is the kind of parallel representation the invariants keep warning about. INVARIANTS INVARIANTS
The .dag modeling quality is not good enough on the new fold ownership path. 04_emit_info.dag adds owned_bindings: Map<String, Bool>. That is a lossy representation: it is keyed by binding name, not binding identity, so it cannot preserve shadowing/scope. The invariants explicitly say a boundary fact table is invalid if it collapses distinct bindings or drops witnesses needed downstream. INVARIANTS languages.dag adds fold_accumulator_shared: Bool, which is a plausible target-language knob, but it is not the actual proof the emitter needs. The real question is not just “does this target share fold accumulators?”, it is “for this specific fold body and this specific accumulator binding, do we have an exact ownership witness that allows move semantics?” The current change still answers that second question with Rust-specific emitter logic, so the new LanguageSpec field does not make the design properly data-driven. The repo’s current guidance is that emit should consume boundary facts rather than rediscover them, and recent emission-debt notes make the same point: missing semantic facts belong in LanguageSpec/boundaries, not in emitter heuristics. Pasted markdown Pasted markdown More broadly, the repo’s compositional direction is “resolve → infer → emit, no re-derivation.” The recent fact-composition notes explicitly say emit should read composed facts and never re-resolve or re-derive them. This PR goes the opposite direction by adding another emitter-local proof pass. 211 211
Real bug: name collision in the init expression. In emit_rust_fold_method_call, once acc_unwrap is true, the PR builds fold_emit_info by inserting the fold accumulator param name into owned_bindings. But that same fold_emit_info is then used not just for the fold lambda, but also for emitting the init expression. Because owned_bindings is only Map<String, Bool>, any outer binding with the same name as the fold accumulator parameter can be misclassified as “owned” while emitting init:. A very ordinary pattern like fn f(acc: Outer, xs) { xs |> fold(init: use(acc.field), f: (acc, x) => ...) } can therefore change field emission on the outer acc, even though it was never unwrapped. This is a real shadowing bug, not just an architectural concern. (Uploaded diff: 05_emit_rust.dag / stage0 mirror around the fold_emit_info construction and init_str emission.) Real bug class: owned_bindings collapses shadowed bindings. Because the new fact is keyed only by String, any inner let acc = ... or nested lambda parameter named acc will inherit the “owned” status of the fold accumulator, even when it refers to a different binding. That can cause wrong move emission on unrelated values. This is exactly why name-keyed semantic tables are dangerous. The safety proof is too narrow to trust. collect_acc_field_moves only understands direct acc.field accesses and only exempts a narrow special case: the first argument of functions in rt_ref_map_functions(). It does not appear to reason about whole-accumulator uses, aliases, method-call borrow contexts, future runtime helpers, or other by-ref paths. A false negative here merely misses the optimization; a false positive changes emitted ownership semantics. Given that asymmetry, the heuristic needs to be much stronger before it is safe. The body-shape proof is underpowered. fold_body_constructs_acc_struct only accepts terminal ExprRecordLit with terminal.name == acc_type_name. That is a brittle, name-based shape test. It will miss valid structured cases (false negatives), and more importantly it is still using a local string/shape heuristic where an upstream ownership fact should exist.
The new tests only grep generated Rust for Rc::try_unwrap and for the presence/absence of .clone() on a field. They do not: compile the emitted Rust, exercise shadowing/name-collision cases, check nested lets / nested lambdas, verify whole-accumulator uses, verify method-call borrow contexts, or prove the intended asymptotic behavior. For a change that alters move-vs-clone semantics, source-string assertions are not enough. The repo’s testing philosophy distinguishes structure-only checks from value/computation checks; this change needs at least one stronger compile-or-run level regression test. INVARIANTS
ROADMAP.md is not modified in this diff, so there is no checkbox audit to do. Indirectly, though, this PR is not aligned with the repo’s current direction. Recent notes already identify “finish EmitContext/boundary migration → emit consumes, not rediscovers” as the right direction, and they frame current emitter heuristics as design debt, not a place to keep adding more logic. This PR adds more emitter-side rediscovery instead of moving the proof upstream. Pasted markdown (2) Pasted markdown
I’d split this into two pieces: Keep the narrow stage0 perf cleanup if desired: the direct Rc::try_unwrap substitutions in the local fold states (SummaryResult, KahnState, etc.) look mechanically reasonable, though they still conflict with the documented no-fallback rule and don’t encode the bound structurally. Drop or rework the fold-emitter ownership optimization. If fold accumulators are really linearly threaded, the proof should come from ownership/infer as an exact, scope-aware witness keyed by binding identity, and emit should merely consume that witness. Right now it is the emitter proving its own exception by local AST inspection, which is exactly the pattern the invariants say not to use. INVARIANTS 211 So my bottom line is: good performance goal, but the current implementation violates repo invariants and has at least one concrete shadowing bug. |
Integrate main's SharingStrategy clone templates (clone_value, deref_clone, iter_owned) and try_target_primitive_type alongside our fold_accumulator_shared and owned_bindings optimization. StoredField now uses apply_type_template1 for the clone path. Stage0 regenerated via two-pass bootstrap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ChatGPT ReviewTwo blockers: src/v2/05_emit_rust.dag: the new Rc::try_unwrap rewrite breaks the emitter’s type invariant. After emit_typed_fold_lambda shadows acc with owned T, only emit_typed_field_access consults owned_bindings; every other acc use still emits as if the binding were the original Rc. fold_body_constructs_acc_struct / fold_body_safe_field_moves do not rule out those non-field uses, so bodies that still pass acc to another helper, capture it in a nested lambda, etc. can pass the new checks and then emit ill-typed Rust. The .dag modeling here is not compositional: owned_bindings is a flat name map on EmitGraphInfo, and fold_emit_info is threaded beyond the exact let acc = Rc::try_unwrap(...) scope (including nested subexpression emission). That means “owned” status is no longer lexical. A reused binding name in a nested fold/lambda can accidentally inherit the outer fold’s owned semantics and get spurious move field accesses / Rc::try_unwrap even though it is a different binding. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (3)
MODELING — Strengths
src/v2/04_emit_info.dagKeeping emission-only ownership context onEmitGraphInfoinstead of pushing Rust policy into core IR is the right layer boundary.src/v2/languages.dagMoving fold-sharing policy intoLanguageSpecis the right layering move because target-language behavior belongs in language data, not Rust-only branches.
MODELING — Improvements
src/v2/04_emit_info.dagThis would compose better if ownership facts were keyed by structural binding identity fromownership.dagrather thanStringnames at the emit boundary.src/v2/languages.dagfold_accumulator_sharedis too coarse to faithfully model sharing semantics; the behavior should emerge from the existing sharing/type-rendering authority rather than a standalone Bool.
ROADMAP — Incomplete
- CG-1 Sharing and ownership: The PR adds one LanguageSpec knob and a Rust-only fold fast path, but single-authority codegen is still unmet because ownership/share decisions are re-derived in emit and partly patched only in stage0.
The PR targets a real performance problem, but it still fixes it in the wrong layer by adding a new codegen flag, new emitter-side ownership heuristics, and new generated-stage0-only optimizations instead of surfacing the needed ownership fact from the upstream authority.
| import_path: String | ||
| } | ||
|
|
||
| type SharingStrategy { |
There was a problem hiding this comment.
Invariant violation: fold_accumulator_shared introduces a global boolean codegen switch, violating Engineering Standards "No flags in codegen" and "Emission is translation, not decision-making" because fold ownership strategy is not derived from structural facts.
| node.children |> flat_map(c => collect_acc_field_moves(node: c, acc_name: acc_name)) | ||
| } | ||
| _ => | ||
| node.children |> flat_map(c => collect_acc_field_moves(node: c, acc_name: acc_name)) |
There was a problem hiding this comment.
Invariant violation: Branching on rt_ref_map_functions() inside collect_acc_field_moves re-derives borrow/move semantics by helper name in the Rust emitter, violating Root-Cause Depth, Heuristics indicate lost structure, and No parallel implementations; this ownership fact should come from ownership.dag.
| @@ -103,13 +103,13 @@ v2_rt::rc_map_insert(acc.clone(), m.name.clone(), exported_set.clone()) | |||
| let resolve_accum = modules.clone().iter().cloned().fold(Rc::new(ResolveAccum { | |||
There was a problem hiding this comment.
Invariant violation: This Rc::try_unwrap optimization pattern, repeated in sibling src/v2/stage0/ files in this diff, patches generated Rust without any corresponding src/v2/*.dag authority, violating No parallel implementations and the hand-editable generated code escape hatch.
# Conflicts: # src/v2/05_emit_rust.dag # src/v2/stage0/src/v2_compiler_emit_rust.rs # src/v2/tests/src/pipeline.rs
The boolean was always false across all targets, making cond_sharing a dead branch. Fold ownership eligibility is fully determined by structural checks (Rc-wrapped, struct repr, body constructs acc, safe field moves) — no codegen flag needed. Addresses review violation: "No flags in codegen" / "Emission is translation, not decision-making." 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: 2, MODELING: 2+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (2)
MODELING — Strengths
src/v2/04_emit_info.dagThe new emit-boundary fact is at least consumed in the same PR, so it does not introduce speculative metadata with no downstream reader.src/v2/languages.dagKeeping target-specific sharing syntax insideSharingStrategystays at the intended LanguageSpec parameterization layer.
MODELING — Improvements
src/v2/04_emit_info.dagModel accumulator ownership by structural binding identity from ownership analysis rather thanMap<String, Bool>keyed by names, because ownership is a fact about bindings, not strings.src/v2/languages.dagfold_accumulator_sharedis not a language fact or algebraic structure; it is a fold-specific optimization policy and should emerge from ownership/sharing authorities instead of a global boolean inLanguageSpec.
ROADMAP — Incomplete
- CG-1 Sharing and ownership: The PR targets fold clone cost, but it does not advance the roadmap’s single-authority design because sharing and ownership still live in Rust-emitter heuristics and generated-stage0 patches instead of TypeRendering/ValueContext or ownership facts.
The PR addresses a real performance symptom, but the implementation is still downstream compensation in emit/stage0 rather than the upstream ownership and sharing authority required by the invariants and roadmap.
| concat("|", params_str, "| ", body_str) | ||
| } | ||
| _ => emit_typed_expr(texpr: lambda_expr, registry: registry, scope: scope, depth: depth, rc_types: rc_types, emit_info: emit_info, fuel: 1024) | ||
| } |
There was a problem hiding this comment.
Invariant violation: Adding fold_terminal_expr and collect_acc_field_moves creates a new emit-side ExprData walk to rediscover fold ownership facts, violating Root-Cause Depth, No parallel implementations, and "Emission is translation, not decision-making"; this authority belongs in ownership/reconcile, not the Rust renderer.
| ExprLambda { semantics: _ } => match lambda_param_names(texpr: fold_lambda_node) |> first { | ||
| Some { value: n } => n | ||
| None => "" | ||
| } |
There was a problem hiding this comment.
Invariant violation: The new acc_unwrap decision is a renderer-local heuristic bundle (cond_sharing, cond_struct, cond_body, cond_safe) rather than an upstream proof, violating Heuristics indicate lost structure and "Emission is translation, not decision-making".
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (2)
MODELING — Strengths
src/v2/04_emit_info.dagKeeping emission inputs in a singleEmitGraphInfoboundary record composes with the existing boundary-contract pattern better than scattering new globals or side caches.
MODELING — Improvements
src/v2/04_emit_info.dagowned_bindings: Map<String, Bool>is not a faithful or compositional ownership model because it uses binder names as identity proxies and encodes an emitter-local decision rather than a structural fact from ownership analysis.
ROADMAP — Incomplete
- PERF: eliminate O(N²) HashMap cloning in complexity analysis: The diff improves generated/stage0 clone behavior and Rust emit heuristics, but it does not add the upstream
src/v2/*.dagownership or complexity authority needed to show the O(N²) class is eliminated at the source level.
The PR targets a real performance problem, but the remaining new work still fixes it in emit/generated artifacts instead of the upstream authority and adds non-behavioral tests that lock in the workaround.
| Some { value: summary } => match summary.repr { StructRepr => true _ => false } | ||
| None => false | ||
| } | ||
| let cond_body = fold_body_constructs_acc_struct(lambda_node: fold_lambda_node, acc_type_name: acc_type_name) |
There was a problem hiding this comment.
Invariant violation: emit_rust_fold_method_call fabricates owned_bindings inside the emitter instead of consuming an upstream ownership fact, violating Explicit boundary contracts, the "new semantic boundaries must land end-to-end" rule, and Emission is translation, not decision-making.
| assert!( | ||
| content.contains("Rc::try_unwrap(acc)"), | ||
| "fold should emit Rc::try_unwrap for struct accumulator, got:\n{}", | ||
| content |
There was a problem hiding this comment.
Invariant violation: This test asserts emitted Rust substrings like Rc::try_unwrap(acc) instead of observable compiler behavior, violating Testing Invariants "Behavioral only" and creating a tautological codegen lock-in.
… blocker detail - Tests: 294→316 pass, 41→44 ignored - Hand-maintained files: 9→2 (7 eliminated in PR #316) - Expand depth/indent asymmetry blocker with resolution options - Record PR #338 review feedback (MethodTemplateSpec, method name enum) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
cache_summary/lookup_summaryredirected to athread_local! RefCell<HashMap>cache, eliminating O(N²) HashMap cloning fromRc::make_mut()on every insertRc<CostInternTable>through every call (no structural changes), but it's now a no-op passthroughcomplexity.dagchange deferred to CX trackRoot cause: Every
cache_summary()call created a newRc<CostInternTable>wrapping anrc_map_insert()of the cloned inner HashMap. Since the table Rc was always shared (refcount >1 due to threading through fold chains andcost_of_exprrecursion),Rc::make_mut()triggered a full HashMap clone on every insert.Fix: 3 cache access points (
cache_summary,lookup_summary, final extraction inbuild_complexity_report) now use a thread-localRefCell<HashMap>instead. Zero type changes, zero derive changes.Test plan
cargo test -p v2-compiler-tests— 275 pass, 0 failcargo clippy --all-targets -- -D warnings— cleancomplexity_report_scales_to_large_programs(was disabled for OOM — this fix should unblock it)🤖 Generated with Claude Code