feat(cborlite): a light CBOR reader. - #3935
Conversation
|
Claude finished @RafaelGranza's task in 9m 27s —— View job Review:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## perf/cbor/primitives #3935 +/- ##
========================================================
+ Coverage 74.71% 74.81% +0.09%
========================================================
Files 463 469 +6
Lines 41066 41433 +367
========================================================
+ Hits 30683 30997 +314
- Misses 8284 8313 +29
- Partials 2099 2123 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| tag, consumed, ok := Tag(data) | ||
| if !ok { | ||
| return 0, errShape | ||
| } | ||
|
|
||
| concrete, known := tagTypes.Load(tag) | ||
| if !known { | ||
| return 0, errShape | ||
| } | ||
| concreteType := concrete.(reflect.Type) | ||
| if !reflect.PointerTo(concreteType).Implements(interfaceType) { | ||
| return 0, errShape | ||
| } | ||
|
|
||
| read, err := cachedReader(concreteType, strict) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("tag %d (%s): %w", tag, concreteType, err) | ||
| } |
There was a problem hiding this comment.
Per-item reflection work on the interface path, which is the hottest path this PR targets.
For every interface-typed item decoded (i.e. every transaction, every trie node) this does:
tagTypes.Load(tag)— async.Maplookup,reflect.PointerTo(concreteType)— internally anothersync.Maplookup inreflect's ptr-type cache,.Implements(interfaceType)— a method-set walk,cachedReader(concreteType, strict)— a thirdsync.Maplookup with acacheKeystruct boxed intoany.
All four results are a pure function of (tag, interfaceType, strict), which are all fixed once the reader is built. Resolving them once per tag and memoising into a sync.Map (or a copy-on-write map[uint64]reader) owned by this closure would cut the steady-state cost to a single lookup, and would also make the "declines a shape it does not know" path cheap.
Given the package's whole premise is "visit a byte once, less CPU", it seems worth not paying Implements per item.
| func specialTypeReader(valueType reflect.Type) (reader, bool) { | ||
| // Implements UnmarshalCBORPrefix | ||
| if reflect.PointerTo(valueType).Implements(prefixUnmarshalerType) { | ||
| return readPrefixUnmarshaler, true | ||
| } | ||
|
|
||
| // Implements UnmarshalBinary | ||
| if reflect.PointerTo(valueType).Implements(binaryUnmarshalerType) { | ||
| return readBinaryUnmarshaler, true | ||
| } | ||
|
|
||
| if valueType == bigIntType { | ||
| return readBigInt, true | ||
| } | ||
|
|
||
| return nil, false | ||
| } |
There was a problem hiding this comment.
cbor.Unmarshaler is not recognised, and the consequence is a silent wrong read rather than a decline.
specialTypeReader honours PrefixUnmarshaler and encoding.BinaryUnmarshaler, but not fxamacker's cbor.Unmarshaler — which is the interface every custom-encoded type in this repo currently implements. A type whose MarshalCBOR writes a shape unrelated to its struct layout gets read structurally here, and in non-strict mode an all-unknown-keys map decodes to a zero value with no error.
Today the two such types happen to be safe by coincidence rather than by construction:
core.TransactionVersion(core/transaction.go:199) marshals as a felt, and since it's a defined type overfp.Elementits Go kind is[4]uint64— soarrayReaderhappens to read the same bytes.core.discardedCBORisstruct{}, soplan.readerdemands a map header and errors out.
Neither is guaranteed. The moment someone adds a MarshalCBOR to a struct with exported fields, or changes TransactionVersion's encoding, this decoder starts returning zero values without complaining — and Unmarshal has no fallback at the field level, so the caller never learns.
Given the package's own invariant ("it must never report success and hand back something else", registry_test.go:50), the safe default is to refuse to build a structural reader for a type where reflect.PointerTo(t) implements cbor.Unmarshaler but not PrefixUnmarshaler. That turns a future silent corruption into a build error that says "add an UnmarshalCBORPrefix", which is exactly the pattern felt.Felt and felt.Slice follow in this PR.
RafaelGranza
changed the title
Reflection over a struct becomes a plan of readers, built once per type and cached. RegisterType now returns the tag it assigned, so the registry teaches both decoders the same tag to type mapping from one list.
A felt packs into an array of limbs, and walking that generically costs a call per limb. Without this the new decoder is slower than the generic one on a class: 396 against 334 us.
3201d88 to
e3e936b
Compare
1 participant
cborlitehas two main goals:Skipping is cheap. No hidden byte checks. Less CPU usage.
This unmarshaler is not exhaustive. It declines a shape it does not know.
A caller can fall back to the generic decoder. Add support when a shape needs it.
It does not check that the bytes are well formed, so it is for bytes this node wrote and read back. Do not use it for external inputs.
To unmarshal a new type, implement
PrefixUnmarshaler.Comparing to
fxmacker.fxmackerneeds us to unmarshal ALL keys inside a SBOR object. Which created our partial objects. Withcborlitewe can actaully skip the keys we don't want.fxmackertraverses every byte at least twice, to check if it's well formed than to decode. In sama cases, for types that don't say its size,fxmackertraverse the bytes a third time to count the byte offset.cborliteonly visits a byte ONCE.Order to review
decode.gocache.goreaders_kind.goplan.goThe other files' order is less relevant.
Speedup
How to measure here. But see following PRs using
cborlite.cborlite coverage by bucket
Tested every populated bucket in a synced mainnet node with deprecated states. Tested on a random 10% population for each bucket larger than 100k. Tested all entries in the smaller ones.
For the buckets of new state, I did a static analysis of the types.
We have 2 unsupported types, and they are not accepted gracefully, with descriptive errors.
The caller can call the generic encoder afterwards. They are mapped in TODOS.
*Specialized Codec means it has its own methods to unmarshal itself, like felts and tries do.