Skip to content

feat(cbor): add RegisterDecoder for per-type decoders - #4206

Open
danielntmd wants to merge 1 commit into
mainfrom
danielntmd/cbor-wrapper
Open

danielntmd wants to merge 1 commit into
mainfrom
danielntmd/cbor-wrapper

Conversation

@danielntmd

@danielntmd danielntmd commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

This PR does not change decoding behavior and only introduces the public API for integrating with a fast CBOR decoder, which follows in the next PRs.

  • Adds cbor.RegisterDecoder[T], which makes a function the decoder of *T and **T destinations.

  • Unmarshal and UnmarshalFirst use the decoder registered for the destination's type, and fxamacker for every other type.

  • Adds a logger and uses fxamacker as the fallback.

  • No tests in this PR in order to avoid unit tests and mocked behavior. Tests through the public API will be introduced downstream when the types are registered.

On the safety of the unsafe pointer*:
decoders.go looks a destination up by the runtime type held in its interface. The lookup is by exact type, so the pointer inside the interface is always the type its decoder was written for, and the casts only erase and restore that type. It assumes gc's layout of an interface, which reflect also relies on.
It saves:

  • about 3% on the smallest records for the type lookup, against map[reflect.Type],
  • up to 12% on a header for the zero check, against reflect's IsZero,
  • about 1% on average for the function cast, against a closure.

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @danielntmd's task in 4m 17s —— View job


Review complete

Summary: Clean design overall — the unsafe-pointer approach is well-reasoned and the PR description already documents the primary risk (eface layout assumption). Two important points for the author to consider, both about undocumented assumptions baked into utils/cbor/decoders.go, not correctness bugs per se.

Important:

  • decoders.go:28-37 — decoderFor silently falls back to fxamacker whenever the destination isn't already zeroed. This is a real perf cliff for any caller that reuses a destination variable across calls, and it's undocumented on RegisterDecoder itself, so a future reader could easily "fix" it away as dead logic.
  • decoders.go:105 — valueDecoder reinterprets a func([]byte, *T) (...) as func([]byte, unsafe.Pointer) (...) via a function-value cast, relying on *T and unsafe.Pointer sharing calling-convention representation. This is a second, separate unsafe assumption beyond the eface-layout one called out in the PR description, worth the same level of documentation (and reconsidering given it's only ~1% of the measured gain).

No blockers or nits beyond the above.

Comment thread utils/cbor/cbor.go Resolved
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit e15e2e7)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
⚠️ Risk level: Medium
📂 Priority files

  • utils/cbor/decoders.go
  • utils/cbor/cbor.go
🏅 Score: 72
🧪 No relevant tests
🔒 No security concerns identified

Comment thread utils/cbor/decoders.go Resolved
Comment thread utils/cbor/decoders.go Resolved
@codecov

codecov Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 23.72881% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.61%. Comparing base (20eaa1f) to head (e15e2e7).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
utils/cbor/decoders.go 21.56% 40 Missing ⚠️
utils/cbor/cbor.go 28.57% 5 Missing ⚠️

❌ Your patch check has failed because the patch coverage (23.72%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4206      +/-   ##
==========================================
- Coverage   81.94%   80.61%   -1.34%     
==========================================
  Files         512      510       -2     
  Lines       39122    39119       -3     
==========================================
- Hits        32060    31536     -524     
- Misses       7053     7574     +521     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- RegisterDecoder registers a decoder for *T and **T destinations. Unmarshal and UnmarshalFirst
  use it, and fxamacker for every other type.
- The decoder is chosen from the destination's type. A nil or non-zero destination goes to
  fxamacker, which merges into it.
- A decoder that fails is logged (SetLogger, sampled) and the item is decoded again with fxamacker.
- Registration is for init only, and a second decoder for a type panics.
- The table is keyed by the interface's type word to skip hashing, which relies on gc's layout of
  an interface, as reflect does.

No decoder is registered yet, so nothing changes until one is.
@danielntmd
danielntmd force-pushed the danielntmd/cbor-wrapper branch from e1a6b5b to e15e2e7 Compare October 27, 2026 00:32
@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @danielntmd's task in 10m 46s —— View job


Review complete

Summary: This PR wires RegisterDecoder[T] and dispatch via decoderFor, with a new per-decode-error fallback to fxamacker (logged, sampled) that wasn't in the previously reviewed revision. The design is sound and well-reasoned, but two unsafe-pointer assumptions remain undocumented/unguarded since the last review pass on this PR.

Important:

  • cbor.go:63 — RegisterDecoder doesn't reject a nil decode func. It registers successfully and only panics later, at first decode time, far from the actual mistake (init-time registration). Worth a nil check that panics immediately, matching the existing duplicate-registration panic style.
  • decoders.go:33-42 — decoderFor's fast path requires the destination to already be zeroed (isZero gate); otherwise it silently falls back to fxamacker every time. This is a real perf cliff for any caller reusing a destination across calls, and it's not documented on RegisterDecoder or explained here — a future reader could easily mistake it for dead logic and remove it.
  • decoders.go:129 — valueDecoder reinterprets a func([]byte, *T) (...) as func([]byte, unsafe.Pointer) (...) via a func-value unsafe.Pointer cast, relying on *T/unsafe.Pointer sharing ABI representation for every T — a second unsafe assumption beyond the eface-layout one called out in the PR description, and one go vet won't catch if it ever breaks. Per the PR's own numbers this only saves ~1%, so a plain typed wrapper closure would likely cost about the same and remove the extra unsafe surface.

No blockers. These are the same two root issues flagged in the prior automated review (nil-decoder gap and the undocumented zero-destination gate), still present on top of the new fallback-to-fxamacker logging behavior added since then, plus the func-cast concern restated at its new line number.
· branch danielntmd/cbor-wrapper

@github-actions

Copy link
Copy Markdown
Contributor

Title

feat(cbor): add RegisterDecoder for per-type decoders


PR Type

Enhancement


Description

  • Add RegisterDecoder[T] for per-type CBOR decoders

  • Dispatch Unmarshal/UnmarshalFirst to registered decoders, fxamacker otherwise

  • Key registry by interface type word via unsafe

  • Failed decodes log sampled warning, then use fxamacker


File Walkthrough

Relevant files
Enhancement
decoders.go
Add type-word keyed decoder registry with fallback             

utils/cbor/decoders.go

  • New file implementing the decoder registry: decoder, typeDecoder, and
    the decoders map
  • Look up decoders by the interface's type word using unsafe casts
    (eface, typeWord, dest), assuming gc's interface layout, skipping map
    hashing
  • valueDecoder (for *T) and pointerDecoder (for **T, mapping CBOR
    null/undefined to a nil pointer) provide decodeUnsafe, reset, and
    isZero behavior
  • On decode failure: reset the destination, emit a sampled warning via
    the atomic logger (set by SetLogger), and fall back to fxamacker
+173/-0 
cbor.go
Route Unmarshal through registered decoders                           

utils/cbor/cbor.go

  • Update package doc to describe registered-decoder dispatch
  • Unmarshal and UnmarshalFirst now route through decoderFor(v) instead
    of calling fxamacker directly; zero or nil destinations still go to
    fxamacker
  • Add generic RegisterDecoder[T] to install a decoder for *T and **T
    destinations; panics on duplicate registration, intended for init-time
    use
+17/-4   
Configuration changes
node.go
Wire CBOR fallback logger at startup                                         

node/node.go

  • Import the new utils/cbor package
  • Call cbor.SetLogger(logger) during New so failed-decode fallbacks are
    reported through the node's logger
+2/-0     

Comment thread node/node.go
if err != nil {
return nil, err
}
cbor.SetLogger(logger)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With metrics this is avoidable, but this logger will be removed when fallback is removed.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e15e2e7

Comment thread utils/cbor/decoders.go Resolved
Comment thread utils/cbor/cbor.go Resolved
Comment thread utils/cbor/decoders.go Resolved
@danielntmd
danielntmd deployed to Development October 27, 2026 00:46 — with GitHub Actions Active
@danielntmd
danielntmd requested a review from EgeCaner October 28, 2026 18:30

This branch was successfully deployed

1 active deployment
Development — e15e2e7a Deployed Oct 27, 2026 by danielntmd via dev_deploy_test / deploy #6185
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant