Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

uses varint encoding for vote-state lockout offsets #27565

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

behzadnouri
Copy link
Contributor

Problem

VoteStateUpdate has large serialized size.

Summary of Changes

The commit removes CompactVoteStateUpdate and instead reduces serialized
size of VoteStateUpdate using varint encoding for vote-state lockout
offsets.

Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

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

Since offset already has known bounds does this save us much vs using fixed? And is there a performance hit when serializing / deserializing varints?

Makes the code cleaner so I think it's a better solution, curious about the metrics.

#[serde(with = "serde_varint")]
offset: Slot,
#[serde(with = "serde_varint")]
confirmation_count: u32,
Copy link
Contributor

Choose a reason for hiding this comment

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

can be u8

Copy link
Contributor Author

@behzadnouri behzadnouri Sep 5, 2022

Choose a reason for hiding this comment

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

done.

any reason the original VoteStateUpdate defines a u32?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was copied over from the original Vote which defined confirmation_count as u32. @carllin was there a specific reason for that?

@carllin
Copy link
Contributor

carllin commented Sep 2, 2022

Cool stuff, would be good to get a size comparison between this and the existing compactvotestate for a full tower (i.e. a vote with 32 slots)

@behzadnouri behzadnouri force-pushed the vote-state-varint branch 3 times, most recently from e773a65 to 0b286a7 Compare September 5, 2022 17:33
@behzadnouri behzadnouri marked this pull request as ready for review September 5, 2022 19:09
@behzadnouri
Copy link
Contributor Author

Since offset already has known bounds does this save us much vs using fixed? And is there a performance hit when serializing / deserializing varints?

yeah, might not save much. It depends on the actual value of offsets.
There is only one lockouts vector so we save the vector length encoding for the other two.
There is also no root_to_first_vote_offset: u64.
Also in cases where 21 bits are enough to encode the offset we save one byte compared to lockouts_32.

On the other hand there are instances we might need an extra byte due to most significant bit being used for varint encoding.
So at the end depends on the distribution of offsets.

The commit also removes compact/uncompact back-and-forth, and simplifies the code a lot. So it might be better in that regard even if the actual bytes savings are not significant.

@behzadnouri behzadnouri force-pushed the vote-state-varint branch 5 times, most recently from df33dfa to c45e3c9 Compare September 6, 2022 15:15
@bw-solana
Copy link
Contributor

bw-solana commented Sep 9, 2022

Collected some data comparing the encoding methods with a validator running on testnet. Here's the average serialized size of vote state for each (sample size of 57k votes observed):

  • VoteStateUpdate = 430B
  • CompactVoteStateUpdate = 176B
  • serde_compact_vote_state = 116B <-- Note my experiment added enum wrapper (extra 4B)

I was seeing very little variation for each. Very sparingly size would dip to the following minimum values

  • VoteStateUpdate = 418B
  • CompactVoteStateUpdate = 174B
  • serde_compact_vote_state = 114B

Data can be found here.

carllin
carllin previously approved these changes Sep 9, 2022
@AshwinSekar AshwinSekar self-requested a review September 9, 2022 22:48
AshwinSekar
AshwinSekar previously approved these changes Sep 9, 2022
@mergify mergify bot dismissed stale reviews from carllin and AshwinSekar September 11, 2022 17:12

Pull request has been modified.

@behzadnouri behzadnouri force-pushed the vote-state-varint branch 5 times, most recently from 8560158 to 64f5df1 Compare September 11, 2022 22:53
  #[derive(Deserialize, Serialize)]
  struct Foo {
    #[serde(with = "serde_varint")]
    field: u64,
    ...
  }
The commit removes CompactVoteStateUpdate and instead reduces serialized
size of VoteStateUpdate using varint encoding for vote-state lockout
offsets.
@behzadnouri behzadnouri merged commit 4f22ee8 into solana-labs:master Sep 12, 2022
@behzadnouri behzadnouri deleted the vote-state-varint branch September 12, 2022 16:31
mergify bot added a commit that referenced this pull request Sep 12, 2022
…#27727)

* adds serde varint encoder/decoder

  #[derive(Deserialize, Serialize)]
  struct Foo {
    #[serde(with = "serde_varint")]
    field: u64,
    ...
  }

(cherry picked from commit f6fbc47)

* uses varint encoding for vote-state lockout offsets

The commit removes CompactVoteStateUpdate and instead reduces serialized
size of VoteStateUpdate using varint encoding for vote-state lockout
offsets.

(cherry picked from commit 4f22ee8)

# Conflicts:
#	core/src/consensus.rs
#	programs/vote/src/vote_instruction.rs
#	programs/vote/src/vote_state/mod.rs
#	sdk/program/src/vote/state/mod.rs

* removes mergify merge conflicts

Co-authored-by: behzad nouri <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants