Skip to content

Fix variant_surfaces double-clone in reconcile_with_typed_cache - #6996

Merged
briansrls merged 1 commit into
mainfrom
bench-b-only
Jul 21, 2026
Merged

briansrls merged 1 commit into
mainfrom
bench-b-only

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Isolated Fix B for whole-tree compile speed (bold-crane-271): in reconcile_with_typed_cache, stop holding two live Rcs across rc_map_insert by building variant_surface first, then moving variant_surfaces into the insert.

Previously each module slot did rc_map_insert(variant_surfaces.clone(), …, build_variant_export_surface(…, variant_surfaces.clone(), …)), keeping refcount ≥ 2 and forcing Rc::make_mut structural copies on every im::HashMap update in the reconcile fold.

M1 cross-batch resolve memo is already on main — not in this PR.

Benchmark (local, compile_clean_diagnostic_histogram, whole-tree)

Variant HISTOGRAM_ELAPSED_SECS reconcile gantt
main (302e955b78) 188.5 150.0s
B-only (this PR) 192.8 152.6s

Run-to-run variance on this box (~2%) swamps the micro-fix; the change removes a proven refcount≥2 call-site pattern regardless.

Test plan

  • cargo fmt --all --check
  • compile_clean_diagnostic_histogram whole-tree — HISTOGRAM_STATUS ok, HISTOGRAM_TOTAL_HARD 0
  • CI build job green
  • CI floor (ci) — pending at mark-ready time

@gunbai-bot gunbai-bot Bot changed the title Whole-tree compile speed: rc_map_insert quadratic + M1 resolve memo Fix variant_surfaces double-clone in reconcile_with_typed_cache Jul 21, 2026
@gunbai-bot
gunbai-bot Bot marked this pull request as ready for review July 21, 2026 13:42
@gunbai-bot

gunbai-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Verified review 40762 against f96c323 — finding holds, no code change needed.

What changed: reconcile_with_typed_cache now builds variant_surface first, then moves variant_surfaces into rc_map_insert instead of rc_map_insert(variant_surfaces.clone(), …, build_variant_export_surface(…, variant_surfaces.clone(), …)).

Semantics: build_variant_export_surface only reads the prior map (no in-place mutation); computing the surface before insert sees the same pre-insert state as before. Net effect is one fewer Rc<im::HashMap> clone per module slot in the reconcile fold — the insert path no longer holds two live Rcs across Rc::make_mut.

Verdict: Agree with APPROVE — narrow mechanical cost-shape fix, identical behavior.

— sent from bold-crane-271

@gunbai-bot

gunbai-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Verified review 40769 against f96c323 — finding holds, no code change needed.

The diff matches the described shape exactly:

  • build_variant_export_surface reads the pre-insert map via variant_surfaces.clone() at cli_run.rs:6645
  • variant_surfaces is then moved into rc_map_insert at 6648–6649 (no second clone on the insert path)

build_variant_export_surface is read-only over surfaces (v1_compiler_infer.rs:16070+); reordering compute-before-insert preserves behavior while dropping one Rc clone per reconcile slot. Agree this is a valid §6 cost-shape fix with no modeling surface.

— sent from bold-crane-271

@briansrls
briansrls merged commit 74e503c into main Jul 21, 2026
4 of 6 checks passed
@briansrls
briansrls deleted the bench-b-only branch July 21, 2026 15:34
gunbai-bot Bot pushed a commit that referenced this pull request Jul 21, 2026
Drops stale cli_run.rs two-dot noise that re-forced whole-tree floor OOM
on 2dbb295; PR contribution unchanged (3-file .dag strip).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant