Skip to content

perf(rt): restore reduce's ArrayVector and Range fast paths - #686

Merged
mparrett merged 3 commits into
mainfrom
wt/fix-reduce-shadow
Aug 8, 2026
Merged

perf(rt): restore reduce's ArrayVector and Range fast paths#686
mparrett merged 3 commits into
mainfrom
wt/fix-reduce-shadow

Conversation

@mparrett

@mparrett mparrett commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

reduce is 1.88x slower than v1.12.2 on main (#656, measured 2026-08-05). The cause is a duplicate registration, not anything in the primitives #639 hoisted.

Root cause

reduce has had two implementations for months:

  • a vm.NewCtxNativeFn closure in pkg/rt/lang.go, carrying an ArrayVector fast path, a Range fast path (direct arithmetic, zero seq/chunk allocation), and a reused two-element argument buffer;
  • the //lg:native Reduce/Reduce3 decls in pkg/rt/native_prims.go, delegating to reduceColl, which has none of that.

Both predate #639. The generated registration used to land in a distinct "clojure.core" namespace instead of canonical "core" — the alias bug #639's first commit fixed — so it was invisible and the closure won the name. Once the alias resolved, the generated registrar took the var root, because it drains after installLangNS. reduce switched to the naive body with no diff to reduce itself and no failing test.

Reducing (range 1000000) through that body allocates 66.5 MB against 17.5 MB: a fresh []vm.Value{acc, elem} per element, plus an ArrayChunk per 32 elements via ChunkedFirst. 33 GC cycles against 6.

TestReduceRangeFastPath stayed green throughout. It asserts results, not which path produced them.

What changed

Both fast paths and the reused buffer move into reduceColl, and the lang.go closure is deleted, leaving one implementation. The general seq path gets the buffer too, which it never had: ec.Invoke does not retain its argument slice, so one buffer per reduce call beats one per element.

The rest of the change adds the missing guard. A name registered in both places is a silent hazard: the closure never runs, and nothing stops the two bodies drifting. Every generated registration that lands on a var an earlier registration already interned is now recorded, and a test asserts that set against an accepted list, so adding a //lg:native decl for an already-hand-registered name fails in CI.

The recorder ignores names an earlier generated registration installed. Registration state is process-global, so without that check a repeat registration records its own predecessor as a collision, making the set depend on how many times the registrar ran: go test -count=2 ./pkg/rt reported phantom collisions for the two probe primitives that pkg/rt's own tests bind through the registration path.

Twelve names are in the accepted set: conj, deref, get, int, name, namespace, nth, pop-binding!, push-binding!, some, str, subs. Each is a duplicate implementation awaiting the same port-then-delete treatment. All twelve pairs were compared body for body: the //lg:native decls are faithful ports, several carrying the closure's comments verbatim, so reduce is the only one that had drifted. The duplication is still worth retiring — a second copy nothing executes will drift again — but no other name is silently losing behavior today.

Verification

(reduce + 0 (range 1000000)) on darwin/arm64, hyperfine 3 warmup / 14 runs, confirmed in reversed order (2026-08-05):

build mean GC cycles
pre-#639 (862c7cec) 34.6 ms 6
main 64.9 ms 33
this branch 41.3 ms 9

Every reduce shape improves, not only 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 (pre-#639 parity).

Semantics spot-checked across 2-arg and 3-arg forms, nil and empty collections, vectors, lists, maps, and reduced short-circuit; pkg/rt's existing TestReduceRangeFastPath and TestReduceArrayVectorFastPath cover the ported paths.

Two new tests cover the guard. TestNoNewShadowedHandRegistrations ratchets the accepted set. TestReRegistrationIsNotAHandShadow pins that a repeat generated registration is not a collision; it fails when that check is removed. go test -count=2 ./pkg/rt is green.

make check-generated passes; pkg/rt/native_prims.go feeds the manifest digest, hence the generated.sums bump.

Residual

This closes most of the gap to pre-#639, not all of it, and the remainder is shared with other primitives. On main, some (1.49x), get (1.18x), nth (1.17x), str (1.11x), conj and deref (1.06x) all sit above pre-#639 with no fast-path story of their own. some is clean at #639 and bisects to #645, so at least one further regression is in flight. Measured separately, not in this PR.

@mparrett mparrett added perf-repeat Run the repeat A/B (variance-reduced) perf check perf Run the perf A/B benchmark check labels Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Perf (pkg/vm micros) — base vs head, same runner

Base aeac42d4b86d vs head fa641f763d4c. Anchor-normalized; informational, not a gate.

bench-ratchet — 0 regression(s) > 12.0% budget, 0 missing, 0 new

  • baseline: AMD EPYC 7763 64-Core Processor / go1.26.5 / linux-amd64
  • current: AMD EPYC 7763 64-Core Processor / go1.26.5 / linux-amd64
  • anchor: baseline 1.222 ns/op, current 1.235 ns/op (+1.1%)
Benchmark Baseline× Current× Δ% Wall Best since Status
nothing past the budget threshold

Showing 0 of 53 benchmarks (past budget). Full table in the run summary →

@mparrett

mparrett commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Perf (pkg/vm micros) — base vs head, same runner

Base 6f75229d77ff vs head 2220c61d9fc1. Anchor-normalized; informational, not a gate.

bench-ratchet — 0 regression(s) > 12.0% budget, 0 missing, 0 new

  • baseline: AMD EPYC 9V74 80-Core Processor / go1.26.5 / linux-amd64
  • current: AMD EPYC 9V74 80-Core Processor / go1.26.5 / linux-amd64
  • anchor: baseline 1.091 ns/op, current 1.093 ns/op (+0.2%)

Benchmark Baseline× Current× Δ% Wall Best since Status
nothing past the budget threshold
Showing 0 of 53 benchmarks (past budget). Full table in the run summary →

Might be nice to surface wins here, too. 🤷🏻

@mparrett

mparrett commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Single-shot (EPYC 9V74): 0 of 53 benchmarks past the 12% budget, anchor stable at +0.2%.

Repeat A/B (EPYC 7763, N=7 interleaved ABBA): all 53 families comparable, 0 missing / 0 new / 0 flaky, would-gate empty at 6%, 8% and 10%. Largest median 2.70% (VectorConj/ArrayVector/10); median of all medians −0.42%. The two families that would have shown trouble are flat:

FuncInvoke/Closure +1.80%, FrameDispatch +0.70%.

So the fix is neutral on the VM micros, which is what it should be — it touches pkg/rt, not pkg/vm. And "0 new" confirms the prediction: no reduce family exists, so this run says nothing about the actual win.

One thing worth noting from the raw cycles: single-cycle deltas reach ±10% while every median sits under 3%. Cycle 3 is visibly contaminated — its anchor dropped 1.246 → 1.177 ns and most families spike in that one cycle. The interleaving plus median absorbed it exactly as #445 predicted, which is a decent argument for the repeat lane over the single-shot one.

@mparrett
mparrett requested a review from nnunley August 6, 2026 03:24
@mparrett
mparrett force-pushed the wt/fix-reduce-shadow branch from 2220c61 to 18919d4 Compare August 6, 2026 05:37
@mparrett
mparrett marked this pull request as ready for review August 6, 2026 05:37
@mparrett
mparrett force-pushed the wt/fix-reduce-shadow branch from 170d8b0 to af3f870 Compare August 6, 2026 13:30
@mparrett mparrett added perf-repeat Run the repeat A/B (variance-reduced) perf check and removed perf-repeat Run the repeat A/B (variance-reduced) perf check labels Aug 6, 2026
@mparrett
mparrett requested a review from nooga August 6, 2026 20:46
Comment on lines +23 to +36
var knownShadowedHandRegistrations = []string{
"clojure.core/conj",
"clojure.core/deref",
"clojure.core/get",
"clojure.core/int",
"clojure.core/name",
"clojure.core/namespace",
"clojure.core/nth",
"clojure.core/pop-binding!",
"clojure.core/push-binding!",
"clojure.core/some",
"clojure.core/str",
"clojure.core/subs",
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue is that the conversion to lg native isn't complete yet, so these weren't covered as needed. This should be tracked by #531

@nnunley nnunley left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I think the shadowed hand registered methods are a gap in the migration to lg:native. A fast follow would be to create tracking issues under #531 to complete the migration.

mparrett and others added 3 commits August 7, 2026 12:59
…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>
…ar 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>
…hadow

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>
@mparrett

mparrett commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Filed both under #531:

Also rebased onto main, which cleared the merge conflict. Checks are green.

@mparrett
mparrett merged commit 8a8222d into main Aug 8, 2026
19 checks passed
@mparrett
mparrett deleted the wt/fix-reduce-shadow branch August 8, 2026 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf Run the perf A/B benchmark check perf-repeat Run the repeat A/B (variance-reduced) perf check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants