⚡ perf: reduce allocations and improve latency across schema package#60
Conversation
- builtinConverters: replace map with array indexed by reflect.Kind for O(1) lookup - structInfo.get(): remove redundant linear scan, map is always complete - fieldInfo: cache lowercased alias (aliasLower) computed once in createField - parseTag: use strings.IndexByte instead of strings.Split for first separator - convertPointer: use getBuiltinConverter array lookup instead of switch - isZero: use v.IsZero() + short-circuit evaluation, remove reflect.New overhead - encoder: reorder omitempty check before value computation to skip encoding - isEmptyFields: avoid redundant src[key] lookups and string concatenation - decoder: use getBuiltinConverter array in decode, setDefaults, createField benchstat schema (geomean): -6.2% latency, -12.3% allocs benchstat fiber/v3 (geomean): -12.3% latency across bind benchmarks Key wins: IsZero: -51.6% latency Encode allocs: -55% (11 -> 5 allocs/op) ParseTag B/op: -33% Encode B/op: -13% CheckRequired: -3.7% latency ConvertPointer: -2.7% latency Bind_Query_Map: -37.5% latency (fiber)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several performance optimizations, such as replacing map-based converter lookups with O(1) array lookups, pre-computing lowercase field aliases, and optimizing the isZero helper. Feedback on these changes highlights opportunities to remove the unused encoderFunc field in fieldInfo, avoid a redundant converter call in decoder.go by reusing convertedVal, and eliminate redundant key == path checks in isEmptyFields.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces an O(1) built-in converter lookup and helper, precomputes lowercase field aliases for fast schema lookups, updates decoder/encoder paths to use the new converter API and avoids unnecessary work, and updates tests to match minor expression/style changes. ChangesConverter and cache optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cache.go (1)
173-188:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix alias shadowing when promoting anonymous embedded fields
cache.go’screate()promotion loop callscontainsAlias(others, f.alias)beforeinfo.fieldsByNameis initialized;containsAliaschecksinfo.fieldsByName[aliasKey], so collisions against the outer struct’s own fields are missed during promotion. The promotion loop then appends colliding promoted fields toinfo.fields, anddecoder.go’ssetDefaults/findRequiredFieldsiterate overstruc.fields, so shadowing can break and produce duplicate default/required handling.🐛 Proposed fix: populate
fieldsByNamebefore promotion and keep it current+ info.fieldsByName = make(map[string]*fieldInfo, len(info.fields)) + for _, field := range info.fields { + if _, exists := info.fieldsByName[field.aliasLower]; !exists { + info.fieldsByName[field.aliasLower] = field + } + } for i, a := range anonymousInfos { others := []*structInfo{info} others = append(others, anonymousInfos[:i]...) others = append(others, anonymousInfos[i+1:]...) for _, f := range a.fields { if !containsAlias(others, f.alias) { info.fields = append(info.fields, f) + if _, exists := info.fieldsByName[f.aliasLower]; !exists { + info.fieldsByName[f.aliasLower] = f + } } } } - info.fieldsByName = make(map[string]*fieldInfo, len(info.fields)) - for _, field := range info.fields { - if _, exists := info.fieldsByName[field.aliasLower]; !exists { - info.fieldsByName[field.aliasLower] = field - } - } return info🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cache.go` around lines 173 - 188, The promotion loop in create() uses containsAlias(others, f.alias) before info.fieldsByName is initialized, so collisions with the outer struct's own fields are missed and colliding promoted fields get appended to info.fields; to fix, initialize and populate info.fieldsByName (using aliasLower keys) prior to iterating anonymousInfos and then update info.fieldsByName incrementally as you promote each field from anonymousInfos (ensuring containsAlias and lookups use the up-to-date map); update the logic around anonymousInfos, info.fields and info.fieldsByName so containsAlias sees outer and already-promoted fields to prevent shadowed duplicates used later by decoder.go setDefaults/findRequiredFields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@decoder.go`:
- Around line 210-213: The code calls conv(f.defaultValue) twice causing
redundant work; call getBuiltinConverter(f.typ.Kind()) as before, store the
single conversion result (convertedVal := conv(f.defaultValue)), check
convertedVal.IsValid(), and then use that same convertedVal in vCurrent.Set
instead of calling conv(...) again — update the block around
getBuiltinConverter, convertedVal, and vCurrent.Set to reuse convertedVal.
---
Outside diff comments:
In `@cache.go`:
- Around line 173-188: The promotion loop in create() uses containsAlias(others,
f.alias) before info.fieldsByName is initialized, so collisions with the outer
struct's own fields are missed and colliding promoted fields get appended to
info.fields; to fix, initialize and populate info.fieldsByName (using aliasLower
keys) prior to iterating anonymousInfos and then update info.fieldsByName
incrementally as you promote each field from anonymousInfos (ensuring
containsAlias and lookups use the up-to-date map); update the logic around
anonymousInfos, info.fields and info.fieldsByName so containsAlias sees outer
and already-promoted fields to prevent shadowed duplicates used later by
decoder.go setDefaults/findRequiredFields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a3449f1-b87b-4622-a0b4-4db1e8163732
📒 Files selected for processing (6)
cache.goconverter.godecoder.godecoder_test.goencoder.goutil_test.go
📜 Review details
🔇 Additional comments (11)
util_test.go (1)
154-164: LGTM!decoder_test.go (2)
231-231: LGTM!Also applies to: 247-247, 255-255, 267-267, 286-286, 294-294, 312-312, 320-320, 331-331, 339-339, 347-347, 358-358, 366-366, 374-374
1029-1029: LGTM!Also applies to: 1045-1045, 1053-1053, 1065-1065, 1084-1084, 1092-1092, 1110-1110, 1118-1118, 1129-1129, 1137-1137, 1145-1145, 1156-1156, 1164-1164, 1172-1172
encoder.go (4)
55-61: LGTM!
62-73: LGTM!
75-76: LGTM!
110-117: LGTM!converter.go (2)
34-78: LGTM!
177-229: LGTM!cache.go (1)
340-345: LGTM!decoder.go (1)
241-243: LGTM!Also applies to: 295-330, 487-487, 614-614
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
- Coverage 97.09% 96.95% -0.14%
==========================================
Files 4 4
Lines 792 789 -3
==========================================
- Hits 769 765 -4
- Misses 12 13 +1
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ReneWerner87
left a comment
ReneWerner87
left a comment
There was a problem hiding this comment.
Please review the comments from our AI agents and mark the ones that have been resolved as “resolved.”
|
@pageton small adjustments |
There was a problem hiding this comment.
Pull request overview
This PR introduces performance-oriented refactors across the schema package’s conversion, caching, encoding, and decoding paths with the goal of reducing allocations and improving end-to-end latency while keeping APIs backward-compatible.
Changes:
- Replaced builtin converter map lookups with an O(1)
reflect.Kind-indexed array accessor (getBuiltinConverter), and simplified pointer conversion to a single lookup + switch. - Optimized cache alias lookups by storing precomputed lowercase aliases and simplifying tag parsing to avoid unnecessary allocations.
- Improved encoder/decoder hot paths (earlier
omitemptyshort-circuiting, faster zero checks, and reduced repeated map/index work in required/empty-field checks).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
converter.go |
Adds array-backed builtin converter lookup and updates pointer conversion to use it. |
cache.go |
Speeds up alias resolution and tag parsing; removes redundant lookup fallback. |
encoder.go |
Optimizes isZero and moves omitempty checks ahead of encoding work. |
decoder.go |
Switches to array-backed converter lookup and optimizes required/empty-field checks. |
decoder_test.go |
Simplifies pointer dereference assertions in nested slice comparisons. |
util_test.go |
Updates isZero tests for function values and benchmarks parse/zero/convert paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…onverter list - Restore removed comment explaining slice-of-structs special case in parsePath - Cache getBuiltinConverter result in slice-defaults loop to avoid redundant lookups - Make builtinConverters map the single source of truth; populate array from it in init()
Summary
Performance optimizations for the
schemapackage — reduces allocations and improves latency across encoding, decoding, and converter paths. All changes are backward-compatible with zero API breaks.What changed
Converter layer (
converter.go)builtinConvertersmap with a[reflect.UnsafePointer + 1]Converterarray for O(1) indexed lookup viagetBuiltinConverter(k)convertPointernow uses single array lookup + type switch instead of 14 separate converter callsCache layer (
cache.go)fieldInfonow stores pre-computedaliasLower, eliminating per-lookuputilstrings.ToLowercalls instructInfo.get()structInfo.get()simplified — removed redundant linear scan fallback (map is always complete)parseTagusesstrings.IndexByte(tag, ',')instead ofstrings.Split(tag, ","), avoiding full slice allocation when only the first element is neededcontainsAliasdoes direct map lookup instead of callingget()per structDecoder (
decoder.go)builtinConverters[kind]map lookups replaced withgetBuiltinConverter(kind)array lookupsisEmptyFieldsoptimized — pre-computespathDot, skips empty values early, removes redundantsrc[key]lookupscheckRequiredreturns early when no required fields existEncoder (
encoder.go)omitemptycheck moved beforeencFunc(fieldValue)— skips encoding entirely when field is emptyisZerorewritten: usesv.CanInterface()+ type assertion instead ofreflect.TypeOf((*zero)(nil)).Elem()+MethodByName("IsZero").Call(); array/struct loops use short-circuitreturn falseinstead ofz = z && ...; default case usesv.IsZero()(Go 1.13+) instead ofreflect.Zero+Interface()comparisonBenchmark results
schema package (benchstat, n=8)
fiber/v3 integration (benchstat, n=6)
Testing
make test)go vetcleangovulncheck— 0 vulnerabilitiesgofumptformatting applied-race -count=3)Summary by CodeRabbit
Refactor
Tests