fix(gnovm): meter GC traversal of large primitive-keyed maps#5884
Draft
omarsy wants to merge 1 commit into
Draft
fix(gnovm): meter GC traversal of large primitive-keyed maps#5884omarsy wants to merge 1 commit into
omarsy wants to merge 1 commit into
Conversation
Collaborator
🛠 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
|
MapValue.VisitAssociated walks the map's MapList in O(N), but the GC visit counter (GCVisitorFn) only increments when it calls vis on a boxed key/value object. For a map whose entries are unboxed primitives (map[int]int etc. — data in TypedValue.N, V == nil), vis is never called for the entries, so an N-entry map was counted as ~1 visit and gcVisitGas charged the flat ~29-gas minimum for an O(N) traversal (gnolang#5883). When visiting a MapValue, count one visit per entry whose key AND value are both unboxed (Key.V == nil && Value.V == nil) — exactly the entries that otherwise contribute zero. Boxed keys/values are already counted when vis reaches them, so object/mixed-keyed maps are unaffected (no gas change). Deterministic (walks MapList in insertion order). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
101c53e to
fb16708
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5883.
Problem
GC visit accounting undercharged the O(N) traversal of a fully-primitive map (
map[int]intetc.).MapValue.VisitAssociatedwalks theMapListin O(N), but the visit counter (GCVisitorFn) only increments whenvisis called on a boxed key/value object. Formap[int]intbothTypedValue.Vare nil (data lives in.N), sovisis never called for the entries — an N-entry map counted as ~1 visit andgcVisitGascharged the ~29-gas flat minimum for an O(N) traversal.Fix
When visiting a
MapValue, count one visit per entry whose key and value are both unboxed (Key.V == nil && Value.V == nil) — exactly the entries that otherwise contribute zero. Boxed keys/values are already counted whenvisreaches them, so object/mixed-keyed maps are unaffected (no gas change). Deterministic (walksMapListin insertion order, not the Govmap).Test
gnovm/pkg/gnolang/gc_map_visit_test.go: a 1000-entrymap[int]intnow yieldsvisitCount = 1001(was 1);gcVisitGas40040 (was 29). Verified red→green (fails on unfixed code).Verification (consensus-gas change)
go build ./gnovm/...+go vet+gofmtclean../gno.land/pkg/sdk/vm -run Gas, full./gno.land/pkg/integration -run TestTestdata(txtar),./gnovm/pkg/gnolang -run Files -test.short— all green. Object/mixed maps get +0, so no gas goldens shift.map[int]int/bool/string,map[[1]int]int,map[int]*S, slice/struct/interface values, nested,interface{}holding primitive-vs-pointer): fully-unboxed entries counted, boxed never double-counted; overflow-safe (~9 orders of magnitude headroom).Note
A fully-primitive map is list-walked twice per GC (once to count, once in
VisitAssociated). Kept surgical to avoid changing gas for object maps; a single-walk form would need threading the count through theVisitorsignature — happy to adjust if you'd prefer that.🤖 Generated with Claude Code