Skip to content

RLP: remove allocations, replace buff with pooled buff#12869

Merged
racytech merged 4 commits intomainfrom
rlp_buffered
Dec 3, 2024
Merged

RLP: remove allocations, replace buff with pooled buff#12869
racytech merged 4 commits intomainfrom
rlp_buffered

Conversation

@racytech
Copy link
Copy Markdown
Contributor

@racytech racytech commented Nov 25, 2024

Preparation for RLP gen package. For now replacing stack allocations with pooled buff, and removing other allocations in RLP encoding logic

@racytech racytech requested review from AskAlexSharov, somnergy and yperbasis and removed request for yperbasis November 25, 2024 07:59
Copy link
Copy Markdown
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

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

Benchmark shows that “stack allocations with pooled buff” - improves speed? Probably reduce?

@racytech racytech marked this pull request as ready for review December 3, 2024 14:45
@racytech
Copy link
Copy Markdown
Contributor Author

racytech commented Dec 3, 2024

Replacing stack allocated slice with sync.Pool does not reduce the speed for sure and in some cases increase the speed of encoding. As an example, Header encoding does not benefit from using pooled buf, but Withdrawal does increase the speed for a few nanoseconds. So it's better not to allocate space, but use the same speed.

_, err := w.Write(buffer[:1])
return err
}
nBytes := byte((nBits + 7) / 8)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can use BitLenToByteLen

@racytech racytech enabled auto-merge (squash) December 3, 2024 18:00
@racytech racytech merged commit 2c8003c into main Dec 3, 2024
@racytech racytech deleted the rlp_buffered branch December 3, 2024 18:26
@somnergy
Copy link
Copy Markdown
Member

@racytech Please back-port to release/2.61

somnergy added a commit that referenced this pull request Jan 27, 2025
…3574)

A recent [RLP related
commit](https://github.com/erigontech/erigon/pull/12869/files#diff-ec3e5ec81bd7c4746ee5d895260767e2282f268684ffc0de73d4a7018975493c)
created a uint256 encoding that assumes len = 32 for the passed in
buffered byte array. However, this PR doesn't fix any inherent issues
with [that approach](#12869)
itself. This PR fixes one issue found in `pectra_devnet_5` that creates a
mismatched recovered signer address.
yperbasis added a commit that referenced this pull request Jan 27, 2025
somnergy added a commit that referenced this pull request Jan 28, 2025
…3574)

A recent [RLP related
commit](https://github.com/erigontech/erigon/pull/12869/files#diff-ec3e5ec81bd7c4746ee5d895260767e2282f268684ffc0de73d4a7018975493c)
created a uint256 encoding that assumes len = 32 for the passed in
buffered byte array. However, this PR doesn't fix any inherent issues
with [that approach](#12869)
itself. This PR fixes one issue found in `pectra_devnet_5` that creates a
mismatched recovered signer address.
somnergy added a commit that referenced this pull request Jan 28, 2025
…3574)

A recent [RLP related
commit](https://github.com/erigontech/erigon/pull/12869/files#diff-ec3e5ec81bd7c4746ee5d895260767e2282f268684ffc0de73d4a7018975493c)
created a uint256 encoding that assumes len = 32 for the passed in
buffered byte array. However, this PR doesn't fix any inherent issues
with [that approach](#12869)
itself. This PR fixes one issue found in `pectra_devnet_5` that creates a
mismatched recovered signer address.
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.

4 participants