All encoding/decoding operations read the type cache to find the
writer/decoder function responsible for a type. When analyzing CPU
profiles of geth during sync, I found that the use of sync.RWMutex in
cache lookups appears in the profiles. It seems we are running into
CPU cache contention problems when package rlp is heavily used
on all CPU cores during sync.
This change makes it use atomic.Value + a writer lock instead of
sync.RWMutex. In the common case where the typeinfo entry is present in
the cache, we simply fetch the map and lookup the type.
This adds support for a new struct tag "optional". Using this tag, structs used
for RLP encoding/decoding can be extended in a backwards-compatible way,
by adding new fields at the end.
This change further improves the performance of RLP encoding by removing
allocations for big.Int and [...]byte types. I have added a new benchmark
that measures RLP encoding of types.Block to verify that performance is
improved.
List headers made up 11% of all allocations during sync. This change
removes most of those allocations by keeping the list header values
cached in the encoder buffer instead. Since encoder buffers are pooled,
list headers are no longer allocated in the common case where an
encoder buffer is available for reuse.
Co-authored-by: Felix Lange <fjl@twurst.com>
* rlp: improve nil pointer handling
In both encoder and decoder, the rules for encoding nil pointers were a
bit hard to understand, and didn't leave much choice. Since RLP allows
two empty values (empty list, empty string), any protocol built on RLP
must choose either of these values to represent the null value in a
certain context.
This change adds choice in the form of two new struct tags, "nilString"
and "nilList". These can be used to specify how a nil pointer value is
encoded. The "nil" tag still exists, but its implementation is now
explicit and defines exactly how nil pointers are handled in a single
place.
Another important change in this commit is how nil pointers and the
Encoder interface interact. The EncodeRLP method was previously called
even on nil values, which was supposed to give users a choice of how
their value would be handled when nil. It turns out this is a stupid
idea. If you create a network protocol containing an object defined in
another package, it's better to be able to say that the object should be
a list or string when nil in the definition of the protocol message
rather than defining the encoding of nil on the object itself.
As of this commit, the encoding rules for pointers now take precedence
over the Encoder interface rule. I think the "nil" tag will work fine
for most cases. For special kinds of objects which are a struct in Go
but strings in RLP, code using the object can specify the desired
encoding of nil using the "nilString" and "nilList" tags.
* rlp: propagate struct field type errors
If a struct contained fields of undecodable type, the encoder and
decoder would panic instead of returning an error. Fix this by
propagating type errors in makeStruct{Writer,Decoder} and add a test.
These changes fix two corner cases related to internal handling of types
in package rlp: The "tail" struct tag can only be applied to the last field.
The check for this was wrong and didn't allow for private fields after the
field with the tag. Unsupported types (e.g. structs containing int) which
implement either the Encoder or Decoder interface but not both
couldn't be encoded/decoded.
Also fixes#19367
The bug can cause crashes if Read is called after EOF has been returned.
No code performs such calls right now, but hitting the bug gets more
likely as rlp.EncodeToReader gets used in more places.
The rules have changed as follows:
* When decoding into pointers, empty values no longer produce
a nil pointer. This can be overriden for struct fields using the
struct tag "nil".
* When decoding into structs, the input list must contain an element
for each field.
The encoder was missing a special case for one element strings whose
element is below 0x7f. Such strings must be encoded as a single byte
without a string header.
This needs to be supported because []someInterface does occur sometimes.
Funny enough, the fix involves changes to the decoder. makeDecoder
cannot return an error for non-empty interfaces anymore because the type
cache builds both decoder and writer. Do the check at 'runtime' instead.
I'm reasonably confident that the encoding matches the output of
ethutil.Encode for values that it supports. Some of the tests have been
adpated from the Ethereum testing repository.
There are still TODOs in the code.
Felix Lange
Marius van der Wijden
Felix Lange
kiel barry
S. Matthew English
Péter Szilágyi
obscuren