Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rlp/rlpgen: RLP encoder code generator #24251

Merged
merged 5 commits into from Feb 16, 2022
Merged

rlp/rlpgen: RLP encoder code generator #24251

merged 5 commits into from Feb 16, 2022

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Jan 19, 2022

This is a new tool to generate EncodeRLP (and DecodeRLP) method implementations from a struct definition.

I have applied this tool to several types in core/types, and here are the benchmark results:

name                             old time/op    new time/op    delta
EncodeRLP/legacy-header-8           328ns ± 0%     237ns ± 1%   -27.63%  (p=0.000 n=8+8)
EncodeRLP/london-header-8           353ns ± 0%     247ns ± 1%   -30.06%  (p=0.000 n=8+8)
EncodeRLP/receipt-for-storage-8     237ns ± 0%     123ns ± 0%   -47.86%  (p=0.000 n=8+7)
EncodeRLP/receipt-full-8            297ns ± 0%     301ns ± 1%    +1.39%  (p=0.000 n=8+8)

name                             old speed      new speed      delta
EncodeRLP/legacy-header-8        1.66GB/s ± 0%  2.29GB/s ± 1%   +38.19%  (p=0.000 n=8+8)
EncodeRLP/london-header-8        1.55GB/s ± 0%  2.22GB/s ± 1%   +42.99%  (p=0.000 n=8+8)
EncodeRLP/receipt-for-storage-8  38.0MB/s ± 0%  64.8MB/s ± 0%   +70.48%  (p=0.000 n=8+7)
EncodeRLP/receipt-full-8          910MB/s ± 0%   897MB/s ± 1%    -1.37%  (p=0.000 n=8+8)

name                             old alloc/op   new alloc/op   delta
EncodeRLP/legacy-header-8           0.00B          0.00B           ~     (all equal)
EncodeRLP/london-header-8           0.00B          0.00B           ~     (all equal)
EncodeRLP/receipt-for-storage-8     64.0B ± 0%      0.0B       -100.00%  (p=0.000 n=8+8)
EncodeRLP/receipt-full-8             320B ± 0%      320B ± 0%      ~     (all equal)

_tmp1 := obj.BaseFee != nil
if _tmp1 {
if obj.BaseFee == nil {
w.Write(rlp.EmptyString)
Copy link
Contributor

@holiman holiman Jan 19, 2022

Choose a reason for hiding this comment

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

This looks like it's unreachable code. Known quirk? Not that it matters a whole lot, if it makes the encoder logic simpler.

Copy link
Contributor Author

@fjl fjl Jan 19, 2022

Choose a reason for hiding this comment

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

Yes, it's unreachable in this case. The double checking happens because BaseFee is optional. If there was another optional field, the check would make sense.

enc.Logs[i] = (*LogForStorage)(log)
func (r *ReceiptForStorage) EncodeRLP(_w io.Writer) error {
w := rlp.NewEncoderBuffer(_w)
w.WriteBytes((*Receipt)(r).statusEncoding())
Copy link
Contributor

@holiman holiman Jan 21, 2022

Choose a reason for hiding this comment

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

This needs to be inside an outer list, doesn't it?

@fjl fjl requested a review from zsfelfoldi as a code owner Jan 24, 2022
@holiman
Copy link
Contributor

holiman commented Jan 25, 2022

triage discussion: this could be fuzzed. We could create types, like headerAlt, which is an exact replica of types.Header. Then instantiate a hdrA headerAlt and hdrB types.Header, populate them identically, and encode - one via reflection and one via generated code, and then we can compare the outputs.

@holiman
Copy link
Contributor

holiman commented Jan 25, 2022

Triage discussion: I'll run this a bit, but also apply the panic on encoding value-types

@fjl
Copy link
Contributor Author

fjl commented Jan 25, 2022

The diff to apply for testing is https://gist.github.com/fjl/10f4b78e54b2ad7113a1b95e3fcaebcd

les/peer.go Outdated
@@ -820,7 +820,7 @@ func (p *clientPeer) sendStop() error {

// sendResume notifies the client about getting out of frozen state
func (p *clientPeer) sendResume(bv uint64) error {
return p2p.Send(p.rw, ResumeMsg, bv)
return p2p.SendItems(p.rw, ResumeMsg, bv)
Copy link
Contributor Author

@fjl fjl Jan 25, 2022

Choose a reason for hiding this comment

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

This change should be reverted because it's a protocol change.

The bug here is: Resume messages are encoded as 0x17 | bv (no list for message content), but the spec says they should be encoded as 0x17 | [ bv ] (with list around bv). Fixing this needs a workaround in the client because existing servers send the wrong encoding.

@holiman
Copy link
Contributor

holiman commented Jan 27, 2022

This has been running for a few days, I've seen no issues with.

fjl added 2 commits Feb 4, 2022
This adds a new package, rlp/internal/rlpstruct, in preparation for the
RLP encoder generator.

I think the struct field rules are subtle enough to warrant extracting
this into their own package, even though it means that a bunch of
adapter code is needed for converting to/from rlpstruct.Type.
This adds new methods on rlp.Stream:

- Uint64, Uint32, Uint16, Uint8, BigInt
- ReadBytes for decoding into []byte
- MoreDataInList - useful for optional list elements
holiman
holiman approved these changes Feb 8, 2022
Copy link
Contributor

@holiman holiman left a comment

LGTM

fjl added 3 commits Feb 16, 2022
This exposes the internal encoder buffer type for use in EncodeRLP
implementations.

The new EncoderBuffer type is a sort-of 'opaque handle' for a pointer to
an internal encBuffer. It is implemented this way to ensure the global
encBuffer pool is handled correctly.
This is a new tool to generate EncodeRLP (and DecodeRLP) method
implementations from a struct definition.
@fjl
Copy link
Contributor Author

fjl commented Feb 16, 2022

I have squashed my commits and have removed the changes to core/types in this PR. They will be resubmitted in another PR.

@fjl fjl merged commit 9b93564 into ethereum:master Feb 16, 2022
1 check passed
@fjl fjl added this to the 1.10.17 milestone Feb 16, 2022
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Feb 18, 2022
This change adds a code generator tool for creating EncodeRLP method
implementations. The generated methods will behave identically to the
reflect-based encoder, but run faster because there is no reflection overhead.

Package rlp now provides the EncoderBuffer type for incremental encoding. This
is used by generated code, but the new methods can also be useful for
hand-written encoders.

There is also experimental support for generating DecodeRLP, and some new
methods have been added to the existing Stream type to support this. Creating
decoders with rlpgen is not recommended at this time because the generated
methods create very poor error reporting.

More detail about package rlp changes:

* rlp: externalize struct field processing / validation

This adds a new package, rlp/internal/rlpstruct, in preparation for the
RLP encoder generator.

I think the struct field rules are subtle enough to warrant extracting
this into their own package, even though it means that a bunch of
adapter code is needed for converting to/from rlpstruct.Type.

* rlp: add more decoder methods (for rlpgen)

This adds new methods on rlp.Stream:

- Uint64, Uint32, Uint16, Uint8, BigInt
- ReadBytes for decoding into []byte
- MoreDataInList - useful for optional list elements

* rlp: expose encoder buffer (for rlpgen)

This exposes the internal encoder buffer type for use in EncodeRLP
implementations.

The new EncoderBuffer type is a sort-of 'opaque handle' for a pointer to
encBuffer. It is implemented this way to ensure the global encBuffer pool
is handled correctly.
qizhou pushed a commit to QuarkChain/go-ethereum that referenced this pull request Mar 4, 2022
This change adds a code generator tool for creating EncodeRLP method
implementations. The generated methods will behave identically to the
reflect-based encoder, but run faster because there is no reflection overhead.

Package rlp now provides the EncoderBuffer type for incremental encoding. This
is used by generated code, but the new methods can also be useful for
hand-written encoders.

There is also experimental support for generating DecodeRLP, and some new
methods have been added to the existing Stream type to support this. Creating
decoders with rlpgen is not recommended at this time because the generated
methods create very poor error reporting.

More detail about package rlp changes:

* rlp: externalize struct field processing / validation

This adds a new package, rlp/internal/rlpstruct, in preparation for the
RLP encoder generator.

I think the struct field rules are subtle enough to warrant extracting
this into their own package, even though it means that a bunch of
adapter code is needed for converting to/from rlpstruct.Type.

* rlp: add more decoder methods (for rlpgen)

This adds new methods on rlp.Stream:

- Uint64, Uint32, Uint16, Uint8, BigInt
- ReadBytes for decoding into []byte
- MoreDataInList - useful for optional list elements

* rlp: expose encoder buffer (for rlpgen)

This exposes the internal encoder buffer type for use in EncodeRLP
implementations.

The new EncoderBuffer type is a sort-of 'opaque handle' for a pointer to
encBuffer. It is implemented this way to ensure the global encBuffer pool
is handled correctly.
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
This change adds a code generator tool for creating EncodeRLP method
implementations. The generated methods will behave identically to the
reflect-based encoder, but run faster because there is no reflection overhead.

Package rlp now provides the EncoderBuffer type for incremental encoding. This
is used by generated code, but the new methods can also be useful for
hand-written encoders.

There is also experimental support for generating DecodeRLP, and some new
methods have been added to the existing Stream type to support this. Creating
decoders with rlpgen is not recommended at this time because the generated
methods create very poor error reporting.

More detail about package rlp changes:

* rlp: externalize struct field processing / validation

This adds a new package, rlp/internal/rlpstruct, in preparation for the
RLP encoder generator.

I think the struct field rules are subtle enough to warrant extracting
this into their own package, even though it means that a bunch of
adapter code is needed for converting to/from rlpstruct.Type.

* rlp: add more decoder methods (for rlpgen)

This adds new methods on rlp.Stream:

- Uint64, Uint32, Uint16, Uint8, BigInt
- ReadBytes for decoding into []byte
- MoreDataInList - useful for optional list elements

* rlp: expose encoder buffer (for rlpgen)

This exposes the internal encoder buffer type for use in EncodeRLP
implementations.

The new EncoderBuffer type is a sort-of 'opaque handle' for a pointer to
encBuffer. It is implemented this way to ensure the global encBuffer pool
is handled correctly.
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Sep 30, 2022
This change adds a code generator tool for creating EncodeRLP method
implementations. The generated methods will behave identically to the
reflect-based encoder, but run faster because there is no reflection overhead.

Package rlp now provides the EncoderBuffer type for incremental encoding. This
is used by generated code, but the new methods can also be useful for
hand-written encoders.

There is also experimental support for generating DecodeRLP, and some new
methods have been added to the existing Stream type to support this. Creating
decoders with rlpgen is not recommended at this time because the generated
methods create very poor error reporting.

More detail about package rlp changes:

* rlp: externalize struct field processing / validation

This adds a new package, rlp/internal/rlpstruct, in preparation for the
RLP encoder generator.

I think the struct field rules are subtle enough to warrant extracting
this into their own package, even though it means that a bunch of
adapter code is needed for converting to/from rlpstruct.Type.

* rlp: add more decoder methods (for rlpgen)

This adds new methods on rlp.Stream:

- Uint64, Uint32, Uint16, Uint8, BigInt
- ReadBytes for decoding into []byte
- MoreDataInList - useful for optional list elements

* rlp: expose encoder buffer (for rlpgen)

This exposes the internal encoder buffer type for use in EncodeRLP
implementations.

The new EncoderBuffer type is a sort-of 'opaque handle' for a pointer to
encBuffer. It is implemented this way to ensure the global encBuffer pool
is handled correctly.
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.

None yet

3 participants