Skip to content

feat(cborlite): add the CBOR decoder primitives - #3934

Draft
RafaelGranza wants to merge 1 commit into
mainfrom
perf/cbor/primitives
Draft

feat(cborlite): add the CBOR decoder primitives#3934
RafaelGranza wants to merge 1 commit into
mainfrom
perf/cbor/primitives

Conversation

@RafaelGranza

@RafaelGranza RafaelGranza commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Header and value decoders that take bytes, unmarshal them and report how many they used, so a caller can keep reading where one left off.

These are the building blocks for faster CBOR unmarshalings. See CBORlite.

We only cover types in use for juno.

@RafaelGranza RafaelGranza self-assigned this Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 6m 4s —— View job


Review: CBOR decoder primitives

Read all five files and checked the decoders against RFC 8949 and against the encoder they have to be compatible with (encoder/encoder.go, fxamacker/cbor in CanonicalEncOptions). No blockers — I found no panic, no out-of-bounds read, and no integer-overflow path. The overflow discipline is genuinely careful: every int(argument) conversion is guarded by a uint64 comparison against the remaining buffer first, the map pair-count doubling divides before it multiplies, and Head rejects additional-info 28–31 so indefinite-length items and the 0xff break byte can't slip through. go vet is clean.

Findings are all coverage and clarity. Posted 6 inline.

Important

  • Tag, Int64 and BigInt have no tests at all — and they're missing from the readers() list in fuzz_test.go too, so the fuzz harness doesn't reach them either. These are the three trickiest readers: BigInt has two decoding paths and two's-complement negation, Int64's MaxInt64 bound is a genuine off-by-one trap, and Tag is the one that matters for interface fields. headers_test.go:153 still carries the doc comment of a deleted cborTag helper — "writes a tag with a four-byte argument, which is what a number this size needs" — which matches tagNum = 65536 in encoder/encoder.go:18. It looks like a test was dropped rather than never written. (inline)
  • Null handling is inconsistent, and BytesNoCopy's doc comment hides it. Bytes and BigInt call ReadNull internally; BytesNoCopy, String, StringNoCopy, Uint64, Int64 and Tag don't — and only Bool/ArrayHeader/MapHeader say so. The doc on BytesNoCopy implies copying is the only difference from Bytes. Swapping one for the other to drop an allocation silently turns every null field into ok == false, with no error message to trace it from. (inline)

Nits

  • BigInt's inline-integer branch is required, not an optimisation — CanonicalEncOptions leaves BigIntConvert at BigIntConvertShortest, so fxamacker writes any *big.Int under 64 bits as a plain major-0/1 integer. And Not rather than Neg deserves a word. Both worth comments so neither gets "simplified" away. (inline)
  • Int64's argument > math.MaxInt64 is correct for both majors and must stay >, since n == MaxInt64 under NegIntMajor is the valid encoding of MinInt64. (inline)
  • Head passes open-ended slices to binary.BigEndian.Uint16/32/64; constant-width slices would help bounds-check elimination on the hottest function in the package. (inline)
  • Four call sites rely on the unwritten invariant that Head's consumed <= len(data); worth stating in its contract. (inline)
  • No benchmarks. The PR is motivated by "faster CBOR unmarshalings" but adds no Benchmark* anywhere under encoder/, so there's no baseline to compare the eventual unmarshaler against fxamacker, and no way to catch a regression in Head/Skip later.
  • There's no float reader — major 7 with info 25/26/27 can be skipped (Head consumes the payload correctly) but not decoded. Fine if no registered type has a float field; worth a line in the package docs either way, since Head hands back the raw IEEE-754 bits as argument and that's an easy thing to misread as a number.
  • String doesn't validate UTF-8. Defensible for a DB-facing decoder and it's what makes it fast — but it means String accepts input fxamacker would reject, so it's a deliberate choice worth recording.
  • Bool returns a literal 1 where the rest of the package uses headerSize; StringNoCopy returns []byte from something named String*; and Bytes shadows its named return in if consumed, isNull := ReadNull(data). All harmless.

I could not run go test ./encoder/cborlite/ — the sandbox declined the command, so the test results above are from reading, not execution. go vet did run and is clean. Happy to implement any of this if you want it pushed.
· branch perf/cbor/primitives

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.67550% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.71%. Comparing base (98ce435) to head (948ce43).

Files with missing lines Patch % Lines
encoder/cborlite/values.go 98.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3934      +/-   ##
==========================================
- Coverage   74.76%   74.71%   -0.05%     
==========================================
  Files         461      463       +2     
  Lines       40915    41066     +151     
==========================================
+ Hits        30590    30683      +93     
- Misses       8232     8284      +52     
- Partials     2093     2099       +6     
Flag Coverage Δ
jsonv2 77.35% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Comment thread encoder/cborlite/headers_test.go Outdated
Comment thread encoder/cborlite/values.go
Comment thread encoder/cborlite/values.go Outdated
Comment thread encoder/cborlite/values.go
Comment thread encoder/cborlite/headers.go
Comment thread encoder/cborlite/headers.go
@RafaelGranza
RafaelGranza marked this pull request as draft August 13, 2026 05:36
Header and value readers that take bytes and report how many they used, so a
caller can keep reading where one left off. No decoder yet.
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