perf(gnovm): speed up DidUpdate per-write ownership hook#5960
Conversation
DidUpdate runs after every mutation of realm-owned state. Profiling a microbenchmark of it showed two dominant costs: PkgID's 20-byte array comparison lowering to runtime.memequal (~18% of samples), and interface dispatch on GetIsReal/GetObjectID/IncRefCount/etc. that the compiler cannot inline. - Add PkgID.eq and rewrite Hashlet.IsZero as hand-unrolled word compares (two uint64 loads + one uint32), used on the per-write hot paths: DidUpdate, IsReadonlyBy, isExternalRealm, and the PushFrameCall borrow-rule check. - Devirtualize DidUpdate: fetch each object's *ObjectInfo once and use concrete, inlinable accessors. Split each Mark* helper into a thin exported wrapper over an unexported markX(oo, oi) body so DidUpdate passes the *ObjectInfo it already holds. No Object type overrides the embedded ObjectInfo methods, so this is behavior-preserving. Behavior and consensus state are unchanged; no gas constants or alloc sizes change. Microbenchmarks (M1 Pro, benchstat n=10, all p=0.000): RealPrimitive -43%, RealAttach -57%, RealSwap -57%, geomean -41%. Adds PkgID.eq / Hashlet.IsZero unit tests (cross-checked against the generic operator per byte), DidUpdate benchmarks, and an ADR that also records why the co==xo and reference-shuffle skips were rejected (ModTime is committed state).
Gno2D2
added
the
review/triage-pending
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
…sled The DidUpdate benchmark helper returned four values and most callers used one, leaving three blank identifiers per call site — tripping the dogsled linter. Return a benchFixture struct so each benchmark reads only the fields it needs. Setup and measured values are unchanged.
…pdate-optimization-46b826
Master's CI now verifies 'go fix' (Go 1.26 modernizer) leaves no diff. Rewrite the segment loops as 'for seg := range HashSize'.
Gno2D2
removed
the
review/triage-pending
There was a problem hiding this comment.
The devirtualization is a real win, but PkgID.eq gives it back on amd64: taking PkgID by value makes every call site copy 20 bytes four times, costing about what the memequal call it removes cost. On 75f126b I ran an interleaved core-pinned A/B on Zen4, benchstat over n=8, same bench file both sides: DidUpdate_RealPrimitive does not move, p=0.855, against a claimed −43.3%. RealAttach and RealSwap do improve, −14.09% and −11.34%. Giving eq a pointer receiver takes the same benchmarks to −46.58% geomean and RealPrimitive to −74.20%; tests green. Your table may well hold on M1, since the copies plausibly forward for free there, but amd64 is what validators run.
[AI bot]
| // per-write hook called on every mutation of realm-owned state. | ||
| // Endianness is irrelevant for equality; NativeEndian loads compile to | ||
| // single MOVs. | ||
| func (pid PkgID) eq(o PkgID) bool { |
There was a problem hiding this comment.
The value receiver costs more than the devirtualization saves: eq copies both 20-byte operands at every call site, four copies at realm.go:341 before a single PkgID comparison. Slicing pid.Hashlet[0:8] forces the operands addressable, so the inlined body cannot compare in registers, and the overlapping 16-byte stores it emits straddle a store boundary on read-back and stall forwarding on amd64. Taking the operands by pointer removes the copies and moves RealPrimitive from p=0.855 to −74.20%.
repro
# from a local clone of gnolang/gno:
gh pr checkout 5960 -R gnolang/gno
cd gnovm/pkg/gnolang
go test -c -o /tmp/head.test .
echo "--- eq symbols surviving linking ---"
go tool objdump -s 'PkgID' /tmp/head.test 2>/dev/null | grep '^TEXT' | grep -i '\.eq'
echo "--- MOVUPS at the real inlined call site (realm.go:341) ---"
go tool objdump -s '\(\*Realm\)\.DidUpdate$' /tmp/head.test | grep -c 'realm.go:341.*MOVUPS'
rm -f /tmp/head.test--- eq symbols surviving linking ---
--- MOVUPS at the real inlined call site (realm.go:341) ---
16
No eq symbol survives linking: it is inlined everywhere, so the clean three-MOV/CMP body that -gcflags=-S prints for the standalone PkgID.eq (size=43, locals=0x0) never executes. That body is copy-free only because the ABI already passes the two 20-byte structs on the stack. At the inlined call site the compiler has to materialize them: the 16 MOVUPS are four 20-byte copies into 0x58(SP), 0x1d4(SP), 0x184(SP), 0x10c(SP), two loads and two stores each. With a pointer receiver the same grep returns 4, and 0 once the intermediate poPkgID local goes too.
| func BenchmarkDidUpdate_NilRealm(b *testing.B) { | ||
| f := benchRealmAndObjects() | ||
| m := benchMachine() | ||
| defer m.Release() | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| nilRealm.DidUpdate(m, f.po, nil, nil) | ||
| } |
There was a problem hiding this comment.
This benchmark never reaches the code the PR changes, so it cannot show −12.8%: benchMachine() leaves Stage at "", and DidUpdate returns on the stage check at realm.go:295, well before the first changed line at 331. The empty Stage also skips the /p/-immutability gate, so the nil-realm shape a real transaction takes goes unexercised.
repro
# from a local clone of gnolang/gno:
gh pr checkout 5960 -R gnolang/gno
cat > gnovm/pkg/gnolang/zz_stage_probe_test.go <<'EOF'
package gnolang
import "testing"
func TestZZStage(t *testing.T) {
m := benchMachine()
defer m.Release()
t.Logf("benchMachine Stage=%q StageRun=%q equal=%v", m.Stage, StageRun, m.Stage == StageRun)
}
EOF
go test -run TestZZStage -v ./gnovm/pkg/gnolang/ 2>&1 | grep 'Stage='
echo "--- first changed line inside DidUpdate (new-file numbering) ---"
git diff -U0 $(git merge-base origin/master HEAD)..HEAD -- gnovm/pkg/gnolang/realm.go | grep '^@@' | sed -n '3p'
rm gnovm/pkg/gnolang/zz_stage_probe_test.go zz_stage_probe_test.go:8: benchMachine Stage="" StageRun="StageRun" equal=false
--- first changed line inside DidUpdate (new-file numbering) ---
@@ -318 +331,8 @@ func (rlm *Realm) DidUpdate(m *Machine, po, xo, co Object) {
The nil-realm block is lines 288-315, so the benchmark returns before line 331.
| // loads: the generic comparison against a zero [20]byte lowers to a | ||
| // runtime.memequal call, and IsZero sits on per-write VM hot paths | ||
| // (ObjectID.IsZero in the readonly/cross-realm guards). | ||
| func (h Hashlet) IsZero() bool { |
There was a problem hiding this comment.
Nit: ObjectID.IsZero reaches Hashlet.IsZero through the same value-receiver chain, so the copies apply here too and the rewrite shows no gain: 9.768n before versus 10.170n after. A pointer receiver fixes it here as well, but changes the method set on an exported type.
| // Fetch each object's *ObjectInfo once: subsequent accessor calls | ||
| // are concrete and inline, where calling through the Object | ||
| // interface would dynamically dispatch on every access. | ||
| poi := po.GetObjectInfo() |
There was a problem hiding this comment.
Missing test: nothing catches an Object implementation overriding an ObjectInfo accessor, which is the assumption the devirtualization rests on. It holds today, but only a grep enforces it, and DidUpdate would silently bypass such an override while every other caller honored it.
test cases
Green at 75f126b; fails if any implementation shadows an accessor. The surface is wider than the var _ Object = ... block suggests: a go/types sweep finds 20 concrete implementations, since the BlockNode family reaches ObjectInfo via StaticBlock -> Block. Full file, with the refcount and owner checks elided here: tests/realm_devirt_test.go
func allObjectImpls() []Object {
return []Object{
// carry ObjectInfo directly (the `_ Object = &X{}` block in ownership.go)
&ArrayValue{}, &StructValue{}, &FuncValue{}, &BoundMethodValue{},
&MapValue{}, &PackageValue{}, &Block{}, &HeapItemValue{},
// reach ObjectInfo via StaticBlock -> Block
&BlockStmt{}, &FileNode{}, &ForStmt{}, &FuncDecl{},
&FuncLitExpr{}, &IfCaseStmt{}, &IfStmt{}, &PackageNode{},
&RangeStmt{}, &SelectCaseStmt{}, &SwitchClauseStmt{}, &SwitchStmt{},
}
}
func TestObjectInfoAccessorsAreNotOverridden(t *testing.T) {
t.Parallel()
for _, oo := range allObjectImpls() {
oi := oo.GetObjectInfo()
if oo.GetObjectInfo() != oi {
t.Errorf("%T: GetObjectInfo() not identity-stable", oo)
}
pid := PkgIDFromPkgPath("gno.land/r/devirt")
oi.SetObjectID(ObjectID{PkgID: pid, NewTime: 7})
if oo.GetObjectID() != oi.ID {
t.Errorf("%T: GetObjectID() != oi.ID", oo)
}
flags := []struct {
name string
set func(bool)
viaO, viaI func() bool
}{
{"IsReal", func(b bool) {
if b {
oi.SetNewTime(7)
} else {
oi.SetNewTime(0)
}
}, oo.GetIsReal, oi.GetIsReal},
{"IsDirty", func(b bool) { oi.SetIsDirty(b, 42) }, oo.GetIsDirty, oi.GetIsDirty},
{"IsEscaped", oi.SetIsEscaped, oo.GetIsEscaped, oi.GetIsEscaped},
{"IsNewReal", oi.SetIsNewReal, oo.GetIsNewReal, oi.GetIsNewReal},
{"IsNewEscaped", oi.SetIsNewEscaped, oo.GetIsNewEscaped, oi.GetIsNewEscaped},
{"IsNewDeleted", oi.SetIsNewDeleted, oo.GetIsNewDeleted, oi.GetIsNewDeleted},
{"IsDeleted", oi.SetIsDeleted, oo.GetIsDeleted, oi.GetIsDeleted},
}
for _, f := range flags {
for _, want := range []bool{true, false} {
f.set(want)
if f.viaO() != f.viaI() {
t.Errorf("%T: Get%s() via Object = %v, via *ObjectInfo = %v",
oo, f.name, f.viaO(), f.viaI())
}
}
}
}
}| @@ -449,18 +483,22 @@ func (rlm *Realm) MarkDirty(oo Object) { | |||
| } | |||
|
|
|||
| func (rlm *Realm) MarkNewDeleted(oo Object) { | |||
There was a problem hiding this comment.
Nit: MarkNewDeleted has no callers left; its only one now calls the unexported markNewDeleted instead. The other three wrappers keep callers, and no linter flags an exported function.
| return binary.NativeEndian.Uint64(pid.Hashlet[0:8]) == binary.NativeEndian.Uint64(o.Hashlet[0:8]) && | ||
| binary.NativeEndian.Uint64(pid.Hashlet[8:16]) == binary.NativeEndian.Uint64(o.Hashlet[8:16]) && | ||
| binary.NativeEndian.Uint32(pid.Hashlet[16:20]) == binary.NativeEndian.Uint32(o.Hashlet[16:20]) |
There was a problem hiding this comment.
Nit: these three loads read exactly 20 bytes, so eq agrees with == only while PkgID is nothing but its Hashlet; the tests catch HashSize drift, but nothing catches a new field. Adding one fails to build only at the unkeyed literal on realm.go:101, and once that is keyed the build is clean, both tests pass, and alloc.go's sizeof guard stays silent since the field hides in ObjectID's padding, while a == b is false and a.eq(b) is true. IsReadonlyBy and isExternalRealm gate realm identity through eq, so that direction fails open; four lines in the idiom alloc.go:146 already uses pin both drifts:
// PkgID.eq and Hashlet.IsZero hard-code an 8+8+4 layout covering all of PkgID.
var (
_ [unsafe.Sizeof(PkgID{}) - 20]struct{}
_ [20 - unsafe.Sizeof(PkgID{})]struct{}
)| rlm.markNewReal(oo, oo.GetObjectInfo()) | ||
| } | ||
|
|
||
| func (rlm *Realm) markNewReal(oo Object, oi *ObjectInfo) { |
There was a problem hiding this comment.
Suggestion: the four markX(oo, oi) bodies require oi == oo.GetObjectInfo() and nothing checks it. markDirty sets the flag on oi but appends oo to rlm.updated, so a mismatched pair flags one object and enqueues another. Latent while the helpers stay unexported, and debugAssert is a build-tag const so the guard is free in production.
repro
# from a local clone of gnolang/gno:
gh pr checkout 5960 -R gnolang/gno
cat > gnovm/pkg/gnolang/zz_pair_test.go <<'EOF'
package gnolang
import "testing"
func TestZZMarkPair(t *testing.T) {
rlm := NewRealm("gno.land/r/x")
rlm.Time = 1
mk := func(nt uint64) *StructValue {
sv := &StructValue{}
sv.SetPkgID(rlm.ID)
sv.SetNewTime(nt)
return sv
}
a, b := mk(2), mk(3)
rlm.markDirty(a, b.GetObjectInfo()) // mismatched pair
t.Logf("a.GetIsDirty()=%v b.GetIsDirty()=%v", a.GetIsDirty(), b.GetIsDirty())
t.Logf("rlm.updated contains a: %v", len(rlm.updated) == 1 && rlm.updated[0] == Object(a))
}
EOF
go test -run TestZZMarkPair -v -tags debugAssert ./gnovm/pkg/gnolang/ 2>&1 | grep -E 'GetIsDirty|contains|PASS'
rm gnovm/pkg/gnolang/zz_pair_test.go zz_pair_test.go:16: a.GetIsDirty()=false b.GetIsDirty()=true
zz_pair_test.go:17: rlm.updated contains a: true
--- PASS: TestZZMarkPair (0.00s)
The flag lands on b while a is what gets enqueued for saving, and -tags debugAssert stays silent.
| recvOID := obj.GetObjectInfo().ID | ||
| if !recvOID.IsZero() && !recvOID.PkgID.IsStdlibPkg() && | ||
| (m.Realm == nil || recvOID.PkgID != m.Realm.ID) { | ||
| (m.Realm == nil || !recvOID.PkgID.eq(m.Realm.ID)) { |
There was a problem hiding this comment.
Suggestion: borrow rule #2 converts here, but rule #3 keeps != for the same PkgID compare thirty lines down at machine.go:2408, in this same function. Two more expressions of the same shape stay on != at realm.go:710 and realm.go:807, differing from the converted 382 and 404 only in the variable name. Those are finalize-time so leaving them is defensible, but two idioms for one comparison now coexist with nothing saying which to reach for.
| } | ||
| } else { | ||
| if oo.GetOwner() == nil { | ||
| if oi.GetOwner() == nil { |
There was a problem hiding this comment.
Nit: this reads oi.GetOwner() while the *PackageValue branch at realm.go:434 reads pv.GetOwner()/pv.GetRefCount() for the same values. The split invites a reader to think they differ.
3 participants
Summary
Realm.DidUpdateis the ownership hook invoked after every mutation of realm-owned state (allop_assignpaths, inc/dec, map/slice/append writes, pointer writes). CPU-profiling a microbenchmark of it surfaced two dominant, avoidable costs:runtime.memequal(~18% of samples).PkgIDwraps aHashlet [20]byte; thepoPkgID != rlm.IDcomparison lowers to aruntime.memequalcall (Go only unrolls small array compares), paid on every real-object write and again for theco/xosub-cases.DidUpdateand theMark*helpers calledGetIsReal,GetObjectID(a 28-byte struct copy per call),IncRefCount,GetIsDirty, … through theObjectinterface — dynamic calls the compiler cannot inline.MarkDirtyalone made 3–4 per invocation.Changes
Optimization (
gnovm/pkg/gnolang/)realm.go— addPkgID.eq: three inlined word compares (2×uint64+ 1×uint32) replacingmemequal. DevirtualizeDidUpdate(fetch each object's*ObjectInfoonce; concrete inlinable accessors thereafter). Split eachMarkX(oo)into a thin exported wrapper over an unexportedmarkX(oo, oi)body soDidUpdatereuses the*ObjectInfoit already holds. Exported API unchanged.hash_image.go— rewriteHashlet.IsZeroas word-OR instead of== Hashlet{}(samememequalissue; used byObjectID.IsZeroin the readonly/cross-realm guards).ownership.go,machine.go— route the per-write guards (IsReadonlyBy,isExternalRealm, thePushFrameCallborrow-rule check) througheq.Tests
realm_pkgid_test.go—TestPkgIDEq/TestHashletIsZero, cross-checking each helper against the generic operator for every byte position.realm_didupdate_bench_test.go—DidUpdatemicrobenchmarks across the steady-state paths (nil-realm, unreal, primitive write, attach, swap).ADR
gnovm/adr/pr5960_didupdate_hot_path.md— context/decision/alternatives, including why the dormantco == xofast-path and the reference-shuffle skip were rejected (they'd changeModTime, which is amino-serialized into the object image and thus committed consensus state).Correctness
Behavior-preserving.
PkgID.eqandHashlet.IsZeroare byte-identical to the generic operators (all 20 bytes, flag nibble included), verified per-byte by the new tests. The devirtualization is safe because noObjecttype overrides the embeddedObjectInfomethods (verified by grep;*ObjectInfois the sole implementer), sooo.M()≡oo.GetObjectInfo().M(). No gas constants or allocation sizes change.Benchmarks
benchstat, n=10, Apple M1 Pro, controlled A/B on the same benchmark binary:DidUpdate_NilRealmDidUpdate_UnrealDidUpdate_RealPrimitiveDidUpdate_RealAttachDidUpdate_RealSwapGeomean −41%, zero allocs on both sides. Post-change profile shows no
memequaland no non-inlined callees inDidUpdate.Verification
go test ./gno.land/pkg/sdk/vm/ -run Gas✅go test ./gno.land/pkg/integration/ -run TestTestdata✅go test ./gnovm/pkg/gnolang/ -run Files -test.short✅go vet/gofmtclean; builds across gnovm, gno.land, tm2.🤖 AI-assisted (Claude). Human-reviewed before submission.