Repository navigation
perf: skip data-only analysis, wire InternTable, fix merge_envs (~50% speedup) - #422
Conversation
ChatGPT ReviewGrounded in the thesis, the question here is not “did this get faster?” but “did this make performance a consequence of carrying the right facts forward?” The good news is that most of the direction is right: threading I would still leave three blocking comments.
In
The comment says that last branch is only for parser-synthesized What I’d ask for: either restrict that fallback to
In That is exactly the kind of lossy boundary the invariants warn against: if a boundary fact table collapses distinct upstream facts or drops witnesses needed later, it is already an invariant violation, and the fix belongs at the boundary, not in downstream assumptions. The roadmap also still says What I’d ask for: either actually merge the tables here, or add a fail-closed assertion/check that all merged envs share the same table, or narrow the type so mixed-table merges are impossible. As written, the helper’s contract is stronger than the code proves.
The new code is better in one respect: Why this matters in practice: any caller that orders args, fills defaults, or keys a map by authored name can now collapse multiple distinct failures onto the same empty key. That is safer than wrong names, but it is still not “zero bugs by construction.” One non-blocking note: The “skip data-only analysis” part looks like a good tactical ratchet, but I would describe it that way, not as architectural closure. The thesis says the end state is dissolving separate passes into binding-carried facts, and the roadmap still treats ownership’s big remaining win as blocked on Track 3 identity plus LS-4 borrow modeling. So the speedup is real and welcome, but it is still a tactical cut over a still-open structural gap. On compositional modeling quality: I did not see a new Roadmap alignment: The two tests I’d add before merging are: a behavior test proving Verdict: good direction, not merge-ready yet. The speedup is thesis-consistent only if identity becomes more exact at the same time; right now the diff still has two silent recovery channels ( |
![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: dd94ca9c4a
ℹ️ 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".
| " modules.push(m);\n", | ||
| " intern_tables.push(result.intern_table.clone());\n", | ||
| " }\n", | ||
| " let intern_table = crate::v2_std_core::merge_intern_tables(std::rc::Rc::new(intern_tables));\n", |
There was a problem hiding this comment.
In profile_gist_pipeline, the newly added merge_intern_tables(...) value is never consumed (this profile only runs tokenize/parse/resolve and never calls reconcile), but it runs before parse_total is recorded. That means the reported parse stage now includes extra full-table merge work unrelated to parse itself, which skews the profiling output and makes historical perf comparisons for this workflow unreliable.
Useful? React with 👍 / 👎.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · dd94ca9c
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/2!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagThe upstream gap is that synthesized name-bearing nodes still do not carry a single structural name authority (M2/M5/M8), so authored_name_at reconstructs identity from the copied Node.name string and downstream lookup, pattern, and emit code then branches on that proxy. Define one core synthetic-name authority, use it everywhere synthesized names are created including the remaining 04_infer bridge sites, and let authored_name_at return only source- or synthetic-authoritative names, never node.name.src/v2/04_env.dagThe upstream boundary between frontend naming and semantic typing is still missing, so a transit-only parser artifact gets copied into every TypeEnv (M1/M2) and downstream code compensates with guesses like first-env wins and empty_intern_table in emit scopes. Keep InternTable in FrontendResult or a dedicated compile context, then move toward M4 by letting semantic envs consume structural ids or Node references instead of String-plus-table ambient state.
MODELING — Strengths
src/v2/04_infer.dagAnonymous record literals now preserve each inferred field's existing span and ident_span, which composes from prior structure instead of fabricating a fresh zero-span name.
MODELING — Improvements
src/v2/04_env.dagTypeEnv is most faithful as a product of binding facts; keep interning in a separate compile/frontend layer so the environment stays compositional and stage-correct.
ROADMAP — Incomplete
- P1: Skip CX/ownership for data-only modules: The new outer module filter in extract_func_entries and extract_ownership_proofs is redundant with the existing item.body filter, so stages 5 and 6 still traverse every module and the roadmap's 8→3 reduction is not achieved.
- P2: Fix authored_name_at fallbacks: Kernel spans are decoded now, but authored_name_at still reverts to node.name on non-kernel source-index misses, so Node.name deletion is still blocked.
ROADMAP — Unclaimed
- P3: Wire InternTable to TypeEnv: The diff does thread frontend.intern_table through reconcile, typecheck, and env construction, but ROADMAP.md still shows P3 as pending.
The PR moves performance plumbing and synthetic-name handling forward, but it still falls short of the zero-bugs-by-construction thesis because authored identity is not yet single-authority, parser interning leaks across the semantic boundary, and the claimed data-only stage skip is not actually implemented.
| } else { | ||
| // Remaining fallback for parser-synthesized spans with file "". | ||
| // These will migrate to kernel_span in a follow-up PR. | ||
| node.name |
There was a problem hiding this comment.
Invariant violation: authored_name_at still falls back to node.name when a non-kernel ident span misses source_indices, violating No fallbacks that fabricate and leaving Node.name as a second authority for authored identity.
| TypeEnv { bindings: merged_bindings, recursive_types: merged_recursive, recursive_type_set: merged_recursive_set, inductive_fields: merged_inductive_fields, source_indices: merged_source_indices } | ||
| // All envs in a merge share the same intern_table (threaded from the | ||
| // frontend result), so we take the first one rather than re-interning | ||
| // every string, which would be O(N*M) per merge and dominate compile time. |
There was a problem hiding this comment.
Invariant violation: merge_envs resolves duplicated intern_table state by taking envs |> first, violating No duplicate representations and Minimal information per interface.
|
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/2!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves performance plumbing and synthetic-name handling forward, but it still falls short of the zero-bugs-by-construction thesis because authored identity is not yet single-authority, parser interning leaks across the semantic boundary, and the claimed data-only stage skip is not actually implemented. |
Addresses Codex review on PR #423: 1. Reframe merge_envs as boundary/fact-flow violation (INVARIANTS "Facts Flow Forward"), not KF-2/idempotency failure. The fix was boundary discipline, not a missing optimizer. 2. Rewrite Rule 4 from arity heuristic ("N inputs → suspect") to structural authority test ("does one input carry the fact?"). Legitimate folds share the N-input shape — arity can't be the authority. 3. ROADMAP "Done" items → "PR #422 (pending merge)" since the implementation hasn't landed on main yet. 4. KF-2 retained as secondary connection: merge(a,a,a)=a IS algebraically true, so KF-2 would also catch it. But primary classification is boundary, not optimizer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The bot's ChatGPT session has expired. Reviews are paused until re-login. To fix:
The bot will resume automatically on its next cycle. |
…tern (~50% self-compile speedup)
Self-compile: ~60-75s → ~37s. Perf ratchet 75s → 55s.
Four pieces of work, executed as the first segment of the perf throughline
(see THESIS / perf-lane-guide). The biggest win came from profiling (not
from a planned optimization): merge_envs was re-interning the entire
shared intern_table on every module merge — O(N*modules*2) pure waste.
compile.dag extract_func_entries and extract_ownership_proofs now pre-filter
modules whose items all have body == None. 82 of 143 .dag files are pure
type declarations and now skip complexity + ownership analysis entirely.
Output-preserving (item-level filtering already produced zero results) —
this eliminates iteration/orchestration overhead.
authored_name_at() previously had three paths that fell back to node.name.
Two changes remove most of them:
1. Kernel spans are now self-describing. authored_name_at extracts the
name directly from the file key (<kernel:Name> → "Name") via substring,
so synthesized nodes don't need to be pre-registered in source_indices.
2. Type-synthesis sites now use kernel_span(name: X) instead of
no_span() / make_span(0,0): make_container_type, make_map_type,
make_callable_type, make_tuple_type (including "first"/"second" children),
algebra_value_field, algebra_method_field, nominal_type_ref,
synthesize_optional_some_variant ("value"/"Some" nodes), type-param
binding in fn_with_type_params, and build_type_env_unresolved kernels.
The ident_span: None path now returns "" (anonymous nodes). The
file-not-found path returns "" for non-kernel spans, but still falls
back to node.name for parser-synthesized nodes with name_span: no_span().
That residual fallback (~8 sites in 02_parse.dag) will migrate in PR4.
Added intern_table: InternTable field to TypeEnv. Threaded through the
pipeline: compile_sources → reconcile → typecheck → typecheck_modules →
typecheck_module → build_type_env / build_type_env_unresolved. Updated
all ~14 TypeEnv construction sites. Updated test harness
(compiler_tests_rust.dag) and hand-written tests (infer_semantics.rs,
pipeline.rs) to collect/pass intern tables from parse results.
The intern table is now available everywhere inference runs. PR3 will
migrate registries (TypeEnv.bindings, InferScope.locals, etc.) from
Map<String, _> to Map<Int, _> using ident: Int as the key.
Discovered via running the perf ratchet after PR2. Pipeline regressed
from ~60s baseline to 94s. Profiling showed reconcile was 81% of compile
time, with a fixed ~1.1s cost per module regardless of module size
(even 3-item modules took 1.1s).
Root cause: merge_envs was calling merge_intern_tables(envs |> map(env =>
env.intern_table)), which iterates every string in every table via
intern(). Since PR2 threads a single shared intern_table from the
frontend through every TypeEnv, all three envs in a merge
(kernel/import/pre_local) have the same table. Merging is pure waste:
O(modules * 2 merges/module * 3 envs/merge * strings/table).
Fix: take the first env's intern_table since they're all the same.
Measured impact on the gist pipeline:
Reconcile: 9.54s → 140ms (68x)
Per-module env: ~1.1s → ~5ms (200x)
Total pipeline: 11.72s → 2.34s (5x)
Share of total: 81% → 6% (no longer the bottleneck)
compiler_tests_rust.dag already has profile_full_pipeline,
profile_self_compile, and profile_reconcile_per_module tests (all
immediately. No new code needed — this is Lane D complete per the
perf plan.
bootstrap.rs PERF_RATCHET_SECONDS 75 → 55. Last measured dev run
was 37.6s; 55s gives CI ~15s variance headroom. Per the "ratchet
only down" invariant.
- 396 compiler tests pass (cargo test -p v2-compiler-tests)
- 20 hand-written tests pass
- Fixed-point verified (pass 1 == pass 2 regen)
- Diagnostic ratchet unchanged at 353
- cargo clippy --all-targets -- -D warnings clean
- performance_ratchet: 37.6s (under new 55s budget)
- profile_full_pipeline: reconcile now 6% of time (was 81%)
- Lane B PR3: migrate registries Map<String, _> → Map<Int, _> (~30-60 sites)
- Lane B PR4: migrate .name display reads to authored_name_at (~60 sites)
- Lane B PR5: delete Node.name field (~180+ Node literals)
- Lane C: last-use clone elision (blocked on PR5)
- PR1 residual: ~8 parser name_span: no_span() sites in 02_parse.dag
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
1 participant
Summary
First segment of the perf throughline. Self-compile ~60-75s → ~37s (perf ratchet 75s → 55s). Four pieces of work, though the biggest actual win came from profiling, not from a planned optimization:
merge_envswas re-interning the entire shared intern_table on every module merge.Breakdown
Lane A — skip data-only modules (compile.dag)
extract_func_entriesandextract_ownership_proofsnow pre-filter modules whose items all havebody == None. 82 of 143 .dag files are pure type declarations and skip complexity + ownership analysis entirely. Output-preserving — item-level filtering already produced zero results; this eliminates iteration/orchestration overhead.Lane B PR1 — authored_name_at fallbacks (partial)
authored_name_at()had three paths tonode.name. Two are eliminated:Self-describing kernel spans. The file key
<kernel:Name>encodes the name.authored_name_atnow extracts it via substring instead of requiring every synthesized name to be pre-registered insource_indices.Type-synthesis sites use kernel_span.
make_container_type,make_map_type,make_callable_type,make_tuple_type(+first/secondchildren),algebra_value_field,algebra_method_field,nominal_type_ref,synthesize_optional_some_variant(value/Somenodes), type-param binding in resolve,build_type_env_unresolvedkernels.ident_span: Noneand file-not-found paths now return""for non-kernel spans. A residual fallback tonode.nameremains for parser-synthesized nodes withname_span: no_span()(~8 sites in02_parse.dag) — migrates in PR4.Lane B PR2 — wire InternTable to TypeEnv
Added
intern_table: InternTabletoTypeEnv. Threaded through the pipeline:Updated ~14
TypeEnv { ... }construction sites, the test harness (compiler_tests_rust.dag), and hand-written tests (infer_semantics.rs,pipeline.rs) to collect/pass intern tables from parse results.Unblocks PR3 (registry key migration
Map<String, _>→Map<Int, _>).merge_envs intern_table reuse — the biggest actual perf win
Discovered via running the perf ratchet after PR2 landed. Pipeline went from ~60s baseline to 94s. Profiling showed reconcile was 81% of compile time with a fixed ~1.1s per module regardless of size (even a 3-item module took 1.1s).
Root cause: `merge_envs` was calling `merge_intern_tables(envs |> map(env => env.intern_table))`, which iterates every string in every table via `intern()`. Since PR2 threads a single shared `intern_table` from the frontend through every env, all three envs in a merge (kernel/import/pre_local) have the same table. Merging is pure waste: `O(modules × 2 merges × 3 envs × strings/table)`.
Fix: take the first env's `intern_table` since they're all the same.
Gist pipeline measurements (before → after):
Lane D — profile infrastructure
`compiler_tests_rust.dag` already has `profile_full_pipeline`, `profile_self_compile`, and `profile_reconcile_per_module` tests (all `#[ignore]`'d). They worked as-is and surfaced the merge_envs bug immediately. No new code needed — Lane D complete per the perf plan.
Ratchet bump
`bootstrap.rs` `PERF_RATCHET_SECONDS` 75 → 55. Last measured local run was 37.6s; 55s gives CI ~15s variance headroom. Per the "ratchet only down" invariant.
Key takeaways
Still pending (remaining throughline)
Test plan
🤖 Generated with Claude Code