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

Optimize hexadecimal conversions in serde_utils to speed up JSON serialization of byte collections #84

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

weekday-grandine-io
Copy link
Collaborator

See individual commit messages for details.

Generalize some code in eth2_cache_utils and benches to work on collections containing any type.
…alization of byte collections

Slow serialization to JSON was claimed to be the problem that led to 2e92e00.
BlobSidecar serialization is now around 15.4 times faster.
BlobSidecar deserialization is now around 12 times faster.
SignedBeaconBlock serialization is now up to 1.5 times faster.
SignedBeaconBlock deserialization is now up to 1.1 times faster.

faster-hex might be faster than const-hex in some cases, but faster-hex does not provide the API we require.
See https://crates.io/crates/const-hex/1.14.0 for benchmark results comparing various hexadecimal conversion crates.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@povi povi left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -339,7 +340,6 @@ hash_hasher = '2'
hashlink = '0.9'
hex = { version = '0.4', features = ['serde'] }
Copy link
Member

Choose a reason for hiding this comment

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

thought: const-hex claims to be a drop-in replacement for the hex crate, maybe we should drop hex and switch to const-hex for the rest of the app? Though this might be out of scope for this MR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You summarized my thought process, including the part about scope.

Another reason1 why I'm reluctant to do it is that the richer API of const-hex might allow for something better than a drop-in replacement.

Footnotes

  1. A bad one.

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