Reduce allocations in AVP decode and message encode hot paths#229
Conversation
|
Thanks for the work on this. |
kulcsartibor
force-pushed
the
main
branch
3 times, most recently
from
11b1c5a to
bcdf7bf
Compare
|
I made few changes to the The |
|
Nice work, tests pass locally and the perf motivation is clear. A few things before I approve: 1.
Prefer the first unless you have a reason against it. 2. Split between 3. Sentinel error wrapping is inconsistent 4. 5. Minor
Main ask is #1. Once that's resolved I'll do a final pass and merge. |
|
For the #2 // diam/dict/util.go — current
func (p *Parser) FindAVPWithVendor(appid uint32, code interface{}, vendorID uint32) (*AVP, error) {
switch codeVal := code.(type) {
case string:
// ...
case uint32:
// ...
case int:
// ...
default:
return nil, fmt.Errorf("Unsupported AVP code type %T(%#v)", codeVal, code)
}
}Problems:
Implement it like type AVPCode interface {
uint32 | string
}
func FindAVPWithVendor[T AVPCode](p *Parser, appid uint32, code T, vendorID uint32) (*AVP, error) {
switch c := any(code).(type) {
case uint32:
return p.findByCode(appid, c, vendorID)
case string:
return p.findByName(appid, c, vendorID)
}
// Unreachable: the constraint rules out everything else at compile time.
return nil, fmt.Errorf("dict: unreachable")
} |
|
#1 is clean, thanks. Verified locally with behavioral tests on top of your branch: One subtlety: direct writes to #2 Agreed, v5 material. Generic constraint #3 Still worth doing in this PR; 2-line change. Either wrap all three sites or drop #4 Fine to ship as-is. A one-line comment on Once #3 is fixed, please squash the eight commits into a single commit with a clean message (subject under 72 chars, body describing the perf wins + |
|
Fixed and pushed the changes in squashed commit. |
|
Squash and #3 look good. Two small things before I merge:
Once those are in, I'll merge on green CI. |
Reduces ReadMessage allocations by ~34% and latency by ~26% on the hot
decode path through five targeted changes:
- Add typed `dict.FindAVPByCode(appid, code, vendorID uint32)` to
eliminate `interface{}` boxing and the type switch in the inner decode
loop. The existing `FindAVPWithVendor` is preserved for v4
compatibility; migration to the typed primitive is left as v5 follow-up.
- Pre-merge inherited AVPs into child app indices at `Load()` time, so
every `FindAVPByCode` lookup resolves in a single map access instead
of walking the parent chain at runtime. Bounded duplication: base
AVPs are copied per declared child app at load time, not per message.
- Replace map-based decoder dispatch with an array-indexed table for
built-in TypeIDs (0..maxTypeID). The `Decoder` map remains
pre-populated with every built-in for back-compat; iteration and
direct reads continue to return the library defaults. Direct writes
to `Decoder[builtinType]` are silently no-op'd by `Decode` (the
array wins) — documented; use `RegisterDecoder` to override built-ins.
- Add `RegisterDecoder(t, f) error` for installing custom decoders or
overriding built-ins. Built-in writes mirror into both `decoderArray`
(the dispatch table) and `Decoder` (the introspection map) so the
exported map gives an accurate view of what is actually registered.
Returns `ErrCannotUnregisterBuiltin` on a nil DecoderFunc for a
built-in TypeID — the original built-in cannot be recovered, so
silently disabling Decode is refused.
- Pre-allocate sentinel errors on the AVP decode hot path; replace
`fmt.Errorf` formatting overhead with bare sentinels wrapped via
`%w: have N need M` at the truncation sites that carry byte counts.
All four truncation sentinels (`errAVPHeader/Data/Vendor TooShort`)
are now consistently wrapped; `errors.Is` works everywhere.
- Skip the intermediate `make+copy` for Grouped AVP bodies via
`DecodeGroupedFromBytes`, which takes the payload slice directly.
- Use `putUint24` for direct big-endian 3-byte writes in
`AVP.SerializeTo` and `Header.SerializeTo`, eliminating the per-call
3-byte slice allocation. Header serialisation is now zero-alloc.
No wire-format or dictionary-schema changes. New exported API:
`FindAVPByCode`, `DecodeGroupedFromBytes`, `RegisterDecoder`,
`ErrCannotUnregisterBuiltin`. v5 follow-ups tracked separately:
generic `FindAVPWithVendor[T uint32 | string]` to retire the
`interface{}` lookup path; migration of `message.go` and
`pretty_dump.go` callers to `FindAVPByCode`.
|
Updated and squashed the commit. There is an issue with one test: |
|
Rebase and #4 are clean. Verified locally: single commit on top of current main, doc comment now carries the memory note, all tests pass. The Approving and will merge on green CI. |
fiorix
left a comment
fiorix
left a comment
There was a problem hiding this comment.
LGTM. Will merge once CI is green.
Reduces ReadMessage allocations by ~34% and latency by ~26% on the hot
decode path through five targeted changes:
- Add typed `dict.FindAVPByCode(appid, code, vendorID uint32)` to
eliminate `interface{}` boxing and the type switch in the inner decode
loop. The existing `FindAVPWithVendor` is preserved for v4
compatibility; migration to the typed primitive is left as v5 follow-up.
- Pre-merge inherited AVPs into child app indices at `Load()` time, so
every `FindAVPByCode` lookup resolves in a single map access instead
of walking the parent chain at runtime. Bounded duplication: base
AVPs are copied per declared child app at load time, not per message.
- Replace map-based decoder dispatch with an array-indexed table for
built-in TypeIDs (0..maxTypeID). The `Decoder` map remains
pre-populated with every built-in for back-compat; iteration and
direct reads continue to return the library defaults. Direct writes
to `Decoder[builtinType]` are silently no-op'd by `Decode` (the
array wins) — documented; use `RegisterDecoder` to override built-ins.
- Add `RegisterDecoder(t, f) error` for installing custom decoders or
overriding built-ins. Built-in writes mirror into both `decoderArray`
(the dispatch table) and `Decoder` (the introspection map) so the
exported map gives an accurate view of what is actually registered.
Returns `ErrCannotUnregisterBuiltin` on a nil DecoderFunc for a
built-in TypeID — the original built-in cannot be recovered, so
silently disabling Decode is refused.
- Pre-allocate sentinel errors on the AVP decode hot path; replace
`fmt.Errorf` formatting overhead with bare sentinels wrapped via
`%w: have N need M` at the truncation sites that carry byte counts.
All four truncation sentinels (`errAVPHeader/Data/Vendor TooShort`)
are now consistently wrapped; `errors.Is` works everywhere.
- Skip the intermediate `make+copy` for Grouped AVP bodies via
`DecodeGroupedFromBytes`, which takes the payload slice directly.
- Use `putUint24` for direct big-endian 3-byte writes in
`AVP.SerializeTo` and `Header.SerializeTo`, eliminating the per-call
3-byte slice allocation. Header serialisation is now zero-alloc.
No wire-format or dictionary-schema changes. New exported API:
`FindAVPByCode`, `DecodeGroupedFromBytes`, `RegisterDecoder`,
`ErrCannotUnregisterBuiltin`. v5 follow-ups tracked separately:
generic `FindAVPWithVendor[T uint32 | string]` to retire the
`interface{}` lookup path; migration of `message.go` and
`pretty_dump.go` callers to `FindAVPByCode`.
Reduces ReadMessage allocations by ~34% and latency by ~26% on the hot
decode path through five targeted changes:
- Add typed `dict.FindAVPByCode(appid, code, vendorID uint32)` to
eliminate `interface{}` boxing and the type switch in the inner decode
loop. The existing `FindAVPWithVendor` is preserved for v4
compatibility; migration to the typed primitive is left as v5 follow-up.
- Pre-merge inherited AVPs into child app indices at `Load()` time, so
every `FindAVPByCode` lookup resolves in a single map access instead
of walking the parent chain at runtime. Bounded duplication: base
AVPs are copied per declared child app at load time, not per message.
- Replace map-based decoder dispatch with an array-indexed table for
built-in TypeIDs (0..maxTypeID). The `Decoder` map remains
pre-populated with every built-in for back-compat; iteration and
direct reads continue to return the library defaults. Direct writes
to `Decoder[builtinType]` are silently no-op'd by `Decode` (the
array wins) — documented; use `RegisterDecoder` to override built-ins.
- Add `RegisterDecoder(t, f) error` for installing custom decoders or
overriding built-ins. Built-in writes mirror into both `decoderArray`
(the dispatch table) and `Decoder` (the introspection map) so the
exported map gives an accurate view of what is actually registered.
Returns `ErrCannotUnregisterBuiltin` on a nil DecoderFunc for a
built-in TypeID — the original built-in cannot be recovered, so
silently disabling Decode is refused.
- Pre-allocate sentinel errors on the AVP decode hot path; replace
`fmt.Errorf` formatting overhead with bare sentinels wrapped via
`%w: have N need M` at the truncation sites that carry byte counts.
All four truncation sentinels (`errAVPHeader/Data/Vendor TooShort`)
are now consistently wrapped; `errors.Is` works everywhere.
- Skip the intermediate `make+copy` for Grouped AVP bodies via
`DecodeGroupedFromBytes`, which takes the payload slice directly.
- Use `putUint24` for direct big-endian 3-byte writes in
`AVP.SerializeTo` and `Header.SerializeTo`, eliminating the per-call
3-byte slice allocation. Header serialisation is now zero-alloc.
No wire-format or dictionary-schema changes. New exported API:
`FindAVPByCode`, `DecodeGroupedFromBytes`, `RegisterDecoder`,
`ErrCannotUnregisterBuiltin`. v5 follow-ups tracked separately:
generic `FindAVPWithVendor[T uint32 | string]` to retire the
`interface{}` lookup path; migration of `message.go` and
`pretty_dump.go` callers to `FindAVPByCode`.
Reduces ReadMessage allocations by ~34% and latency by ~26% on the hot
decode path through five targeted changes:
- Add typed `dict.FindAVPByCode(appid, code, vendorID uint32)` to
eliminate `interface{}` boxing and the type switch in the inner decode
loop. The existing `FindAVPWithVendor` is preserved for v4
compatibility; migration to the typed primitive is left as v5 follow-up.
- Pre-merge inherited AVPs into child app indices at `Load()` time, so
every `FindAVPByCode` lookup resolves in a single map access instead
of walking the parent chain at runtime. Bounded duplication: base
AVPs are copied per declared child app at load time, not per message.
- Replace map-based decoder dispatch with an array-indexed table for
built-in TypeIDs (0..maxTypeID). The `Decoder` map remains
pre-populated with every built-in for back-compat; iteration and
direct reads continue to return the library defaults. Direct writes
to `Decoder[builtinType]` are silently no-op'd by `Decode` (the
array wins) — documented; use `RegisterDecoder` to override built-ins.
- Add `RegisterDecoder(t, f) error` for installing custom decoders or
overriding built-ins. Built-in writes mirror into both `decoderArray`
(the dispatch table) and `Decoder` (the introspection map) so the
exported map gives an accurate view of what is actually registered.
Returns `ErrCannotUnregisterBuiltin` on a nil DecoderFunc for a
built-in TypeID — the original built-in cannot be recovered, so
silently disabling Decode is refused.
- Pre-allocate sentinel errors on the AVP decode hot path; replace
`fmt.Errorf` formatting overhead with bare sentinels wrapped via
`%w: have N need M` at the truncation sites that carry byte counts.
All four truncation sentinels (`errAVPHeader/Data/Vendor TooShort`)
are now consistently wrapped; `errors.Is` works everywhere.
- Skip the intermediate `make+copy` for Grouped AVP bodies via
`DecodeGroupedFromBytes`, which takes the payload slice directly.
- Use `putUint24` for direct big-endian 3-byte writes in
`AVP.SerializeTo` and `Header.SerializeTo`, eliminating the per-call
3-byte slice allocation. Header serialisation is now zero-alloc.
No wire-format or dictionary-schema changes. New exported API:
`FindAVPByCode`, `DecodeGroupedFromBytes`, `RegisterDecoder`,
`ErrCannotUnregisterBuiltin`. v5 follow-ups tracked separately:
generic `FindAVPWithVendor[T uint32 | string]` to retire the
`interface{}` lookup path; migration of `message.go` and
`pretty_dump.go` callers to `FindAVPByCode`.
Reduces ReadMessage allocations by ~34% and latency by ~26% on the hot
decode path through five targeted changes:
- Add typed `dict.FindAVPByCode(appid, code, vendorID uint32)` to
eliminate `interface{}` boxing and the type switch in the inner decode
loop. The existing `FindAVPWithVendor` is preserved for v4
compatibility; migration to the typed primitive is left as v5 follow-up.
- Pre-merge inherited AVPs into child app indices at `Load()` time, so
every `FindAVPByCode` lookup resolves in a single map access instead
of walking the parent chain at runtime. Bounded duplication: base
AVPs are copied per declared child app at load time, not per message.
- Replace map-based decoder dispatch with an array-indexed table for
built-in TypeIDs (0..maxTypeID). The `Decoder` map remains
pre-populated with every built-in for back-compat; iteration and
direct reads continue to return the library defaults. Direct writes
to `Decoder[builtinType]` are silently no-op'd by `Decode` (the
array wins) — documented; use `RegisterDecoder` to override built-ins.
- Add `RegisterDecoder(t, f) error` for installing custom decoders or
overriding built-ins. Built-in writes mirror into both `decoderArray`
(the dispatch table) and `Decoder` (the introspection map) so the
exported map gives an accurate view of what is actually registered.
Returns `ErrCannotUnregisterBuiltin` on a nil DecoderFunc for a
built-in TypeID — the original built-in cannot be recovered, so
silently disabling Decode is refused.
- Pre-allocate sentinel errors on the AVP decode hot path; replace
`fmt.Errorf` formatting overhead with bare sentinels wrapped via
`%w: have N need M` at the truncation sites that carry byte counts.
All four truncation sentinels (`errAVPHeader/Data/Vendor TooShort`)
are now consistently wrapped; `errors.Is` works everywhere.
- Skip the intermediate `make+copy` for Grouped AVP bodies via
`DecodeGroupedFromBytes`, which takes the payload slice directly.
- Use `putUint24` for direct big-endian 3-byte writes in
`AVP.SerializeTo` and `Header.SerializeTo`, eliminating the per-call
3-byte slice allocation. Header serialisation is now zero-alloc.
No wire-format or dictionary-schema changes. New exported API:
`FindAVPByCode`, `DecodeGroupedFromBytes`, `RegisterDecoder`,
`ErrCannotUnregisterBuiltin`. v5 follow-ups tracked separately:
generic `FindAVPWithVendor[T uint32 | string]` to retire the
`interface{}` lookup path; migration of `message.go` and
`pretty_dump.go` callers to `FindAVPByCode`.
2 participants
PR Summary
Files changed
Committed:
diam/avp.godiam/datatype/decoder.godiam/datatype/decoder_test.godiam/datatype/datatype.godiam/dict/util.godiam/dict/parser.godiam/group.godiam/header.godiam/uintconv.goOptimizations implemented
1. Typed
FindAVPByCode— avoidinterface{}boxing (diam/dict/util.go,diam/avp.go)Added
FindAVPByCode(appid, code, vendorID uint32)as a typed fast-path for the hot decode loop. The existingFindAVPWithVendortakescode interface{}, which boxes theuint32on every call and requires a type switch. The decode path atavp.go:95now calls the typed method directly. The originalFindAVPWithVendoris preserved for backward compatibility.2. Array-indexed decoder dispatch with map fallback (
diam/datatype/decoder.go)Replaced the
map[TypeID]DecoderFunclookup inDecode()with a fixed-size[maxTypeID]DecoderFuncarray. TypeIDs are small contiguous integers (0–18), so array indexing is a single bounds check vs. map hashing.Decode()falls back to the exportedDecodermap for any TypeID the array doesn't cover, preserving the ability for external code to register custom decoders viadatatype.Decoder[myType] = myFunc. TheDecodermap now also includesQoSFilterRuleTypeandIPv6Typeso it stays consistent with the array. Added test cases to verify the registration of custom TypeID and decoder function.3. Skip intermediate Grouped AVP copy (
diam/avp.go,diam/group.go)Grouped AVPs were decoded twice: first
datatype.DecodeGroupedcopied all payload bytes into adatatype.Grouped(make + copy), thendiam.DecodeGroupedimmediately converted it back to[]byteto parse child AVPs. The newDecodeGroupedFromBytestakes the raw payload slice directly, eliminating the intermediate allocation.DecodeFromBytesnow checks the dictionary type before dispatching, short-circuiting the grouped path. The originalDecodeGroupedis preserved as a wrapper for backward compatibility.4. Sentinel errors with contextual wrapping (
diam/avp.go)Replaced 5
fmt.Errorfcalls on error paths inDecodeFromByteswith pre-allocatederrors.Newsentinel variables (errAVPHeaderTooShort,errAVPDataTooShort,errAVPVendorTooShort). Same forerrAVPSerializeNilDatainSerialize/SerializeTo. Removes formatting overhead and allocations on error paths, and reduces function code size which can help compiler inlining/optimization of the surrounding hot path. On the twoerrAVPDataTooShortreturn sites, the sentinel is wrapped withfmt.Errorf("%w: have %d need %d", ...)so debugging retains byte-count context whileerrors.Isidentity is preserved.5.
putUint24direct buffer write (diam/uintconv.go,diam/avp.go,diam/header.go)Added
putUint24(b []byte, n uint32)(package-internal) that writes 3 big-endian bytes directly into a target buffer. Replaced allcopy(b, uint32to24(n))calls inAVP.SerializeTo(1 call) andHeader.SerializeTo(2 calls). Eachuint32to24allocated a 3-byte slice that was immediately copied and discarded. This single change madeHeader.SerializeTozero-allocation.6. Dictionary inheritance for apps with no own AVPs (
diam/dict/parser.go)mergeInheritedAVPsoriginally built its app set only fromp.avpcodekeys, so applications declared in the dictionary with zero AVPs of their own (e.g.<application id="1001" type="acct"/>) were skipped and base AVPs were never inherited into their lookup index.FindAVPByCodewould then miss and returnUnknownAVPs, breaking CER parsing for such apps. The merge now also seeds the app set fromp.appcode, so every declared app gets its ancestor chain processed at load time. FixesTestHandleCER_HandshakeMetadataTCPandTestHandleCER_HandshakeMetadata_CustomIP, which were hanging on<-sm.HandshakeNotify().Benchmark results (Apple M1 Max, 10 cores)
Backward compatibility
All changes are backward-compatible:
FindAVPWithVendor,FindAVP,DecodeGrouped,uint32to24, and theDecodermap are all preserved.Decode()still resolves custom decoders registered viadatatype.Decoder[myType] = myFunc(fallback after the array miss).FindAVPByCode,DecodeGroupedFromBytes.go test ./...).