perf(ir): retain *runtime-defn-ir-cache* for only the current ns - #650
Conversation
mparrett
left a comment
There was a problem hiding this comment.
The body says self-contained to pipeline.lg, but this PR carries two things belonging to its siblings:
compile-def-fn-value— the Lisp half of #647's Go hook. #647 resolvesir.passes.pipeline/compile-def-fn-valueand no-ops when the lookup fails; that var isn't onmain, and it's defined here with no caller inside this PR. So #647 merged alone is inert, and this one merged alone defines a function nothing calls. I verified both directions by building them.test/ir_junk_agreement.lgandtest/ir_and_cond_rpo.lg— headed "#625 fix 1" and "#625 fix 2", the block-junk-agreement and RPO fixes that live in #648. I ran both againstorigin/main: 7 and 9 passing. They're parity tests rather than repros, so they don't refute #648's "could not reproduce" note — but they're named for #648's fixes and belong with them, and their headers need adjusting either way (details on #648).
So the four-way split doesn't decompose the way the descriptions say. All four target main and all four report MERGEABLE, which hides it. Suggested regrouping:
- #647 plus the
compile-def-fn-valuedefn — one landable seam change, with whatever coverage turns out to be possible - #648 plus the two tests above and the baseline rebaseline
- #649 retargeted onto #648's branch
- #650 reduced to the retention change itself
The retention change reads well on its own. The single-ns window is backed by the code rather than only the comment — seed-inline-registry-from-cache reads (get @*runtime-defn-ir-cache* ns-name {}) at pipeline.lg:66 and its only caller passes the current ns at :694 — so entries for other namespaces really are dead weight. The "a require mid-file switches the cache to the dependency's ns and back" caveat is the case I'd have asked about, and naming the cost as a forfeited inline opportunity is the right read. No objection to that part once it's on its own.
59c940f to
8e56a7a
Compare
8e56a7a to
3308ee3
Compare
mparrett
left a comment
There was a problem hiding this comment.
Re-reviewed the isolated cache-window change at the current head. The implementation matches the same-namespace lookup contract, the tests pin same-namespace accumulation and cross-namespace eviction, targeted local tests pass, and all required checks are green. Approved.
3308ee3 to
13de843
Compare
13de843 to
fefeafb
Compare
Extracted from nooga#625 [4/4]. Drops the process-lifetime retention of the runtime defn IR cache to a single-namespace window — a heap/GC tweak, no behavioral change.
fefeafb to
0cad1e6
Compare
Summary
IR pipeline cache retention: retain runtime-defn-ir-cache for only the current namespace.
When enable-inline is on, the cache stashes each defn's built IR for same-ns inline registry seeding. The registry only seeds same-ns, so entries for other namespaces are dead weight. This change makes each seed replace the cache with a single-key map
{current-ns {...}}instead of accumulating across all loaded namespaces.Decomposition Note
This PR has been decomposed per reviewer feedback:
This PR now contains the cache-window retention change only — the isolated performance improvement for same-ns inline caching.
Base & Merge Order
Base:
main@upstreamMerge order: 4th (lands after #648, #649, #647 in stack)
Verification
go build ./...passesgo test ./pkg/irpassesmake check-generatedpasses