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

perf: improve rlp, update Address methods #118

Merged
merged 6 commits into from
Jun 19, 2023
Merged

perf: improve rlp, update Address methods #118

merged 6 commits into from
Jun 19, 2023

Conversation

DaniPopes
Copy link
Member

Motivation

We can use leading_zeros() to determine ahead of time how many bytes we need to skip, instead of iterating over the bytes and counting them.

Assembly diff of <u64 as Encodable>::encode: https://www.diffchecker.com/TxUdi1il/

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@DaniPopes DaniPopes added the enhancement New feature or request label Jun 16, 2023
@DaniPopes DaniPopes requested a review from prestwich June 16, 2023 05:59
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

UTACK

cc @mattsse who is more familiar with this code

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

tests look good, not exactly sure about the address encoding but seems alright

} else if *self < <$t>::from(EMPTY_STRING_CODE) {
out.put_u8(u8::try_from(*self).unwrap());
} else if x < EMPTY_STRING_CODE as $t {
out.put_u8(x as u8);
Copy link
Member

Choose a reason for hiding this comment

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

this check guarantees this

Comment on lines +296 to +300
// minus 1 to account for the list header itself
out[0] = EMPTY_LIST_CODE + len as u8 - 1;

// address header + address
out[1] = EMPTY_STRING_CODE + 20;
Copy link
Member

Choose a reason for hiding this comment

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

i don't know rlp enough to comment on what's going on here

Copy link
Member Author

Choose a reason for hiding this comment

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

It's basically just inlining Header { list: true, len: len - 1 }.encode() and self.encode()
I've added a proptest to verify this

@DaniPopes DaniPopes requested a review from prestwich June 19, 2023 08:02
@DaniPopes DaniPopes changed the title perf: improve rlp uint encoding, fix Address::create perf: improve rlp, update Address methods Jun 19, 2023
@DaniPopes DaniPopes merged commit 81b1cc3 into main Jun 19, 2023
@DaniPopes DaniPopes deleted the dani/better-rlp branch June 19, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants