feat(rt): hoist 222 clojure.core primitives to named //lg:native decls - #639
Conversation
|
The hoist holds up under measurement:
Those 37 are byte-identical to the set #627 produces on its own, so this PR adds none of its own. Your The audit guard doesn't cover the case its commit message claims.
Cheapest way to close the gap is to have the guard also assert one known source-only name resolves after a Every emitted adapter carries a dead arity check. All 224 open with: if len(vs) < 0 {
return vm.NIL, fmt.Errorf("wrong number of args (%d), expected at least 0", len(vs))
}Always false. No correctness impact, since the real checks live in the bodies ( Question on The Nothing here needs to change before #627 is settled, which is where the blocker sits. Tell me which way you're taking |
a0778db to
0d6b2b6
Compare
Rebased onto current
|
147f840 to
c823f74
Compare
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
Requesting changes for one source-bootstrap blocker.
The ordinary lane passes:
go test -short -count=1 ./...
But the bootstrap lane fails on current head c823f74e. Focused repro:
go test -tags bootstrap -run 'TestLGBParity/ns-require-string' -count=1 ./pkg/bytecode
This fails with Can't resolve s/join in this context; the full bootstrap lane cascades through other source namespaces. It also matches the currently failing GitHub Actions build check.
[P1] Preserve the pending source load for registrar-created namespaces (internal/primgen/prims_emit.go:707).
The emitter now calls LookupOrRegisterNSNoLoad("clojure.string"). During rt init that creates the canonical string placeholder, but it does not set nsNeedsLoad. Later RequireNS("clojure.string") reaches LookupOrRegisterNS, sees an existing namespace with needsLoad == false, and returns it without executing string.lg. Only the generated upper-case binding exists, so source-only siblings such as join, capitalize, and blank? never become resolvable.
Please mark a namespace created solely for generated bindings as still needing its real source/bundle load, without re-marking an already-loaded namespace, and add a bootstrap regression covering a source-only sibling such as clojure.string/join.
make check-generated reached its consistency check successfully. I did not find another correctness blocker in the hoisted bodies or registrar generation.
RegisterGeneratedPrimitives (the lginterop-generated //lg:native
registrar) Defs primitives via LookupOrRegisterNSNoLoad("clojure.core"),
but that path — unlike the loading LookupOrRegisterNS — never resolved
the ns alias, so it registered into a DISTINCT "clojure.core" namespace
instead of the canonical "core". Hand-registered primitives masked this
by also Def'ing into the canonical ns via installLangNS; a primitive
hoisted to a pure //lg:native decl has ONLY the generated registration,
so it landed in the wrong namespace and was invisible to core.lg's own
bootstrap compile ("Can't resolve +").
Resolve the alias as the first step of LookupOrRegisterNSNoLoad, matching
the loading variant. This unblocks hoisting native closures in lang.go to
named //lg:native primitives for real stack-trace frames.
Verified: make check-generated OK (bundle + lowered tree unchanged, this
is a Go-only registration fix); clojure.core surface = 807 publics,
byte-identical before/after a proof hoist of + - * /.
…ation
Hoisting native closures in lang.go to //lg:native primitives means a prim
can now have ONLY the generated registration — so a misregistration (wrong
namespace, dropped //lg:name) surfaces as a terminating bootstrap panic
("Can't resolve X") that reports one symbol at a time. Add diagnostics so a
whole batch of newly hoisted primitives is root-caused in one run:
- AuditGeneratedPrimitives(): non-terminating audit returning one line per
generated primitive NOT resolvable in its canonical namespace, including
where the name actually landed (the exact signature of the alias bug fixed
in the parent commit: bound in "clojure.core", absent from "core").
- evalInit bootstrap compile: on failure, append the audit to the panic so
every misregistered primitive is listed at once, not one panic per re-run.
- LG_REGPRIM_DEBUG: per-primitive registration trace (name, requested ns,
canonical ns, landed-canonical) — grep for landed-canonical=false.
- TestAuditGeneratedPrimitivesCleanAfterInit: permanent regression guard that
every generated primitive resolves in its canonical ns (would have caught
the alias bug); the gate to run after each hoist batch.
Go-only; no generated artifacts change (check-generated unaffected).
…ve decls Codemod that lifts the inline anonymous closures registered as clojure.core primitives in pkg/rt/lang.go into named, //lg:native-annotated top-level functions. Three wins: named stack traces (rt.corePlus not installCore.func42), declarative registration (drop ns.Def, lginterop emits from annotations), and the nooga#411 Fn-adapter done as a codemod (no runtime adapter). Only hoists SAFE sites: no genuine enclosing-scope capture (package refs + params are fine; an installCore local like buildArray/asChunked is not) and a discoverable lg-name from ns.Def("name", var). First run on lang.go: 213 safe, 62 capture-blocked (real shared local helpers), 17 no-name. Dry-run by default (report + proposed funcs); -apply rewrites; -only scopes. Converts trailing []vm.Value params to variadic ...vm.Value (lginterop's shape). Analyzer + generator validated (output gofmt-clean, return types preserved); -apply rewrite path implemented but not yet exercised end-to-end.
Lift installCore-local helper closures (buildArray, asChunked, rangeInt, regexSubmatchValue, regexSubmatchVector) to package-level funcs via cmd/hoist-natives -helpers. Unblocks ~14 primitives whose closures captured them, en route to hoisting the native primitives to //lg:native decls.
The registrar in pkg/rt/zz_primitives_generated.go is what lets the runtime boot; a generator that boots the runtime to produce it depends on the very artifact it emits (p0 depending on p1). When a primitive is hoisted out of installLangNS into a //lg:native decl, the runtime cannot boot until this file carries its registration — so the generator must run in exactly that state. - Extract the primitives codegen into internal/primgen, a PURE go/ast tool with no pkg/compiler / pkg/rt import (prims_scan, prims_emit, Generate, ScanNativeNames, KebabCase). - Add cmd/lgprimgen, a thin binary over primgen; cmd/lginterop delegates its -primitives mode to the same package (its compiler import is only for the interop-EDN path). - Makefile: regenerate zz_primitives_generated.go via lgprimgen BEFORE the lgbgen bootstrap that consumes it, breaking the boot cycle. generate.lg calls lgprimgen too. - Fix a latent emit bug: variadic-spread adapters (Fn(vs...)) dropped the closing paren — never exercised until the first //lg:native ...vm.Value primitives. gofmt failures now surface stderr + dump the source.
Harden the codemod so it only lifts closures that are genuinely safe to lift, leaving the rest untouched. Skip / demote a candidate when: - its lg-name is registered under MULTIPLE names (hoisting one //lg:name would drop the others from clojure.core); - its lg-name is already owned by a pre-existing //lg:native decl (duplicate registration, the generated registrar fails to compile); - it is fetched BY lg-name via a .Lookup string arg anywhere in the package, possibly another file, before the generated registrar runs (would be nil); - it is redefined by a stdlib .lg source; the eager core reapply skips it, so a guarded native there would deviate permanently (native-prims-intact? false); - its local var is still referenced by surviving code (a 2nd registration, a helper, a skipped closure body), removing it would leave an undefined ident. Also: strip the dead err-guard(s) left when a hoisted Wrap assignment is cut, emit EXPORTED CoreX names (the gogen_ir lowered tree direct-calls them as rt.Core-prefixed funcs), and reuse internal/primgen scanner so the codemod and lgprimgen agree on what a name is.
Hoisting closures into //lg:native decls adds them to the guarded native-root set, so a primitive shadowed at load (not restored by reapply) trips native-prims-intact? with no clue which one. Add the diagnostics that pinpoint it: - vm.Var.GuardDeviated() + Namespace.AllVars(): read whether a guarded root deviated from its canonical value, and snapshot every interned var. - rt.deviatedGuardedVars() + LG_GUARD_DEBUG: on the off-fast-path branch of NativePrimsIntact(), name the deviated primitive(s) to stderr instead of a bare intact=false. The fast path stays a single atomic load.
Lift the inline anonymous closures registered as clojure.core primitives in lang.go into named, //lg:native-annotated top-level functions (via cmd/hoist-natives). Three wins: real stack-trace frames (rt.CorePlus, not installLangNS.func42); declarative registration (the ns.Def call is dropped, the generated registrar owns it); and the closures narrow toward typed/ec-threaded signatures. Regenerates zz_primitives_generated.go, the bundle, and the sums. 222 closures were classified safe by the codemod; the analysis skipped the rest (multi-name aliases, //lg:native duplicates, .Lookup-by-name, stdlib-.lg redefinitions, surviving-reference captures). Verified: make check-generated OK on both artifacts; clojure.core surface unchanged at 807 publics; native-prims-intact? true at boot and after (require clojure.string); full go test ./... green.
c823f74 to
6162d6c
Compare
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
Rechecked the updated head. The pending-load fix addresses the source-bootstrap blocker: the focused clojure.string/join repro passes, both the ordinary and bootstrap full test lanes pass locally, make check-generated passes, and all current GitHub checks are green. No remaining findings.
…dowed duplicate reduce had two implementations: a NewCtxNativeFn closure in lang.go carrying ArrayVector and Range fast paths plus a reused two-element argument buffer, and the //lg:native Reduce/Reduce3 decls delegating to reduceColl, which had none of them. Both predate #639. The generated registration used to land in a distinct "clojure.core" namespace rather than canonical "core", so it was invisible and the closure won; #639 fixed that alias, and because the generated registrar drains last it now takes the var root. reduce lost the fast paths silently. The naive body allocates a fresh []vm.Value{acc, elem} per element and an ArrayChunk per 32 elements via ChunkedFirst — 66.5 MB against 17.5 MB reducing (range 1000000), and 33 GC cycles against 6. Measured 1.88x slower end to end (#656). Port both fast paths and the reused buffer into reduceColl and delete the closure, so there is one implementation. The buffer is shared by the general seq path too, which never had it: ec.Invoke does not retain its argument slice, so one buffer per reduce call beats one per element. Measured on darwin/arm64, hyperfine 3 warmup / 14 runs, reversed-order confirmed, reducing (range 1000000): pre-#639 34.6 ms 6 GC cycles main 64.9 ms 33 GC cycles this 41.3 ms 9 GC cycles Every reduce shape improves, not just the two with fast paths: over a list 37.1 -> 30.1 ms, over a vector 29.8 -> 25.5 ms, over a map 97.1 -> 90.1 ms. The residual against pre-#639 is not reduce-specific — `some`, `get`, `nth`, `conj`, `deref` and `str` all sit 1.06-1.49x above pre-#639 on main with no fast-path story, so it is separate and still open. Refs #656 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dowed duplicate reduce had two implementations: a NewCtxNativeFn closure in lang.go carrying ArrayVector and Range fast paths plus a reused two-element argument buffer, and the //lg:native Reduce/Reduce3 decls delegating to reduceColl, which had none of them. Both predate #639. The generated registration used to land in a distinct "clojure.core" namespace rather than canonical "core", so it was invisible and the closure won; #639 fixed that alias, and because the generated registrar drains last it now takes the var root. reduce lost the fast paths silently. The naive body allocates a fresh []vm.Value{acc, elem} per element and an ArrayChunk per 32 elements via ChunkedFirst — 66.5 MB against 17.5 MB reducing (range 1000000), and 33 GC cycles against 6. Measured 1.88x slower end to end (#656). Port both fast paths and the reused buffer into reduceColl and delete the closure, so there is one implementation. The buffer is shared by the general seq path too, which never had it: ec.Invoke does not retain its argument slice, so one buffer per reduce call beats one per element. Measured on darwin/arm64, hyperfine 3 warmup / 14 runs, reversed-order confirmed, reducing (range 1000000): pre-#639 34.6 ms 6 GC cycles main 64.9 ms 33 GC cycles this 41.3 ms 9 GC cycles Every reduce shape improves, not just the two with fast paths: over a list 37.1 -> 30.1 ms, over a vector 29.8 -> 25.5 ms, over a map 97.1 -> 90.1 ms. The residual against pre-#639 is not reduce-specific — `some`, `get`, `nth`, `conj`, `deref` and `str` all sit 1.06-1.49x above pre-#639 on main with no fast-path story, so it is separate and still open. Refs #656 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* perf(rt): restore reduce's ArrayVector/Range fast paths, drop the shadowed duplicate reduce had two implementations: a NewCtxNativeFn closure in lang.go carrying ArrayVector and Range fast paths plus a reused two-element argument buffer, and the //lg:native Reduce/Reduce3 decls delegating to reduceColl, which had none of them. Both predate #639. The generated registration used to land in a distinct "clojure.core" namespace rather than canonical "core", so it was invisible and the closure won; #639 fixed that alias, and because the generated registrar drains last it now takes the var root. reduce lost the fast paths silently. The naive body allocates a fresh []vm.Value{acc, elem} per element and an ArrayChunk per 32 elements via ChunkedFirst — 66.5 MB against 17.5 MB reducing (range 1000000), and 33 GC cycles against 6. Measured 1.88x slower end to end (#656). Port both fast paths and the reused buffer into reduceColl and delete the closure, so there is one implementation. The buffer is shared by the general seq path too, which never had it: ec.Invoke does not retain its argument slice, so one buffer per reduce call beats one per element. Measured on darwin/arm64, hyperfine 3 warmup / 14 runs, reversed-order confirmed, reducing (range 1000000): pre-#639 34.6 ms 6 GC cycles main 64.9 ms 33 GC cycles this 41.3 ms 9 GC cycles Every reduce shape improves, not just the two with fast paths: over a list 37.1 -> 30.1 ms, over a vector 29.8 -> 25.5 ms, over a map 97.1 -> 90.1 ms. The residual against pre-#639 is not reduce-specific — `some`, `get`, `nth`, `conj`, `deref` and `str` all sit 1.06-1.49x above pre-#639 on main with no fast-path story, so it is separate and still open. Refs #656 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(rt): ratchet the set of hand registrations the generated registrar shadows A name registered both by hand in installLangNS and by the generated //lg:native registrar is a silent hazard rather than a style problem: the generated registrar drains last, so the hand-written closure never runs, and nothing stops the two bodies drifting. reduce drifted for months and the whole suite stayed green — including a test named TestReduceRangeFastPath, which asserts results, not the path. Record the collision where it already gets detected (setPrimitiveRoot's existing-binding branch) and expose the set, so adding a //lg:native decl for an already-hand-registered name fails here instead of years later in a benchmark. Twelve names are in the accepted set today; each is a duplicate implementation awaiting the same port-then-delete treatment reduce just got. The two bodies agree for the ones spot-checked (get, some); the rest are unaudited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(rt): don't record a re-registered generated primitive as a hand shadow recordShadowedHandRegistration treated any pre-existing var as evidence of a hand-written registration. Registration state is process-global, so the second generated registration of the same ns/name finds the first one's root already interned and records its own predecessor as a collision. That made the ratchet depend on how many times the suite ran: `go test -count=2 ./pkg/rt` failed with phantom collisions for bind.probe.ns/probe-fn and test.reapplyhybrid/prim, both bound by tests that call the registration path directly. The recorder now skips names already present in genPrimBindings — it runs before the current binding is recorded, so a hit there means an earlier generated registration owns the var. TestReRegistrationIsNotAHandShadow pins the distinction; it fails without the check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Stack: #627 (fix) → this → per-package-registrars → provenance-manifest. Bottom of this PR's new work sits on #627; until #627 merges, the diff shows that fix commit too — it drops out once #627 lands.
Lifts the inline anonymous closures registered as
clojure.coreprimitives inlang.gointo named,//lg:native-annotated top-level functions, via a new codemod (cmd/hoist-natives). Three wins: real stack-trace frames (rt.CorePlus, notinstallLangNS.func42); declarative registration; and closures narrow toward typed/ec-threaded signatures.Commits (this feature)
feat(rt): non-terminating diagnostics for generated-primitive registration (audit +LG_REGPRIM_DEBUG+ regression-guard test)feat(cmd): thehoist-nativescodemodrefactor(rt): lift shared native helper closures to package levelfeat(build): runtime-free primitive registrar generator (lgprimgen) — decouples the generator from the runtime it produces (the p0/p1 bootstrap problem)feat(cmd/hoist-natives): safety analysis (5 skip categories: multi-name aliases,//lg:nativedups,.Lookup-by-name, stdlib-.lgredefinitions, surviving-reference captures)feat(vm,rt): guard-deviation diagnostics (LG_GUARD_DEBUG)feat(rt): the 222-primitive hoist + regenerated artifactsVerification
make check-generatedOK (bundle + lowered tree);clojure.coresurface unchanged at 807 publics;native-prims-intact?true at boot and after(require 'clojure.string); fullgo test ./...green.222 of 292 closures were classified safe; the rest were correctly skipped (see the safety-analysis commit). Reviewed per-commit + a whole-branch pass.