290e1d2ff7
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Paul Hauner
|
b0b606dabe |
Use SmallVec for TreeHash packed encoding (#3581)
## Issue Addressed NA ## Proposed Changes I've noticed that our block hashing times increase significantly after the merge. I did some flamegraph-ing and noticed that we're allocating a `Vec` for each byte of each execution payload transaction. This seems like unnecessary work and a bit of a fragmentation risk. This PR switches to `SmallVec<[u8; 32]>` for the packed encoding of `TreeHash`. I believe this is a nice simple optimisation with no downside. ### Benchmarking These numbers were computed using #3580 on my desktop (i7 hex-core). You can see a bit of noise in the numbers, that's probably just my computer doing other things. Generally I found this change takes the time from 10-11ms to 8-9ms. I can also see all the allocations disappear from flamegraph. This is the block being benchmarked: https://beaconcha.in/slot/4704236 #### Before ``` [2022-09-15T21:44:19Z INFO lcli::block_root] Run 980: 10.553003ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 981: 10.563737ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 982: 10.646352ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 983: 10.628532ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 984: 10.552112ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 985: 10.587778ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 986: 10.640526ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 987: 10.587243ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 988: 10.554748ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 989: 10.551111ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 990: 11.559031ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 991: 11.944827ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 992: 10.554308ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 993: 11.043397ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 994: 11.043315ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 995: 11.207711ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 996: 11.056246ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 997: 11.049706ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 998: 11.432449ms [2022-09-15T21:44:19Z INFO lcli::block_root] Run 999: 11.149617ms ``` #### After ``` [2022-09-15T21:41:49Z INFO lcli::block_root] Run 980: 14.011653ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 981: 8.925314ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 982: 8.849563ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 983: 8.893689ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 984: 8.902964ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 985: 8.942067ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 986: 8.907088ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 987: 9.346101ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 988: 8.96142ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 989: 9.366437ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 990: 9.809334ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 991: 9.541561ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 992: 11.143518ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 993: 10.821181ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 994: 9.855973ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 995: 10.941006ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 996: 9.596155ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 997: 9.121739ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 998: 9.090019ms [2022-09-15T21:41:49Z INFO lcli::block_root] Run 999: 9.071885ms ``` ## Additional Info Please provide any additional information. For example, future considerations or information useful for reviewers. |
||
|
Paul Hauner
|
fe52322088 |
Implement SSZ union type (#2579)
## Issue Addressed NA ## Proposed Changes Implements the "union" type from the SSZ spec for `ssz`, `ssz_derive`, `tree_hash` and `tree_hash_derive` so it may be derived for `enums`: https://github.com/ethereum/consensus-specs/blob/v1.1.0-beta.3/ssz/simple-serialize.md#union The union type is required for the merge, since the `Transaction` type is defined as a single-variant union `Union[OpaqueTransaction]`. ### Crate Updates This PR will (hopefully) cause CI to publish new versions for the following crates: - `eth2_ssz_derive`: `0.2.1` -> `0.3.0` - `eth2_ssz`: `0.3.0` -> `0.4.0` - `eth2_ssz_types`: `0.2.0` -> `0.2.1` - `tree_hash`: `0.3.0` -> `0.4.0` - `tree_hash_derive`: `0.3.0` -> `0.4.0` These these crates depend on each other, I've had to add a workspace-level `[patch]` for these crates. A follow-up PR will need to remove this patch, ones the new versions are published. ### Union Behaviors We already had SSZ `Encode` and `TreeHash` derive for enums, however it just did a "transparent" pass-through of the inner value. Since the "union" decoding from the spec is in conflict with the transparent method, I've required that all `enum` have exactly one of the following enum-level attributes: #### SSZ - `#[ssz(enum_behaviour = "union")]` - matches the spec used for the merge - `#[ssz(enum_behaviour = "transparent")]` - maintains existing functionality - not supported for `Decode` (never was) #### TreeHash - `#[tree_hash(enum_behaviour = "union")]` - matches the spec used for the merge - `#[tree_hash(enum_behaviour = "transparent")]` - maintains existing functionality This means that we can maintain the existing transparent behaviour, but all existing users will get a compile-time error until they explicitly opt-in to being transparent. ### Legacy Option Encoding Before this PR, we already had a union-esque encoding for `Option<T>`. However, this was with the *old* SSZ spec where the union selector was 4 bytes. During merge specification, the spec was changed to use 1 byte for the selector. Whilst the 4-byte `Option` encoding was never used in the spec, we used it in our database. Writing a migrate script for all occurrences of `Option` in the database would be painful, especially since it's used in the `CommitteeCache`. To avoid the migrate script, I added a serde-esque `#[ssz(with = "module")]` field-level attribute to `ssz_derive` so that we can opt into the 4-byte encoding on a field-by-field basis. The `ssz::legacy::four_byte_impl!` macro allows a one-liner to define the module required for the `#[ssz(with = "module")]` for some `Option<T> where T: Encode + Decode`. Notably, **I have removed `Encode` and `Decode` impls for `Option`**. I've done this to force a break on downstream users. Like I mentioned, `Option` isn't used in the spec so I don't think it'll be *that* annoying. I think it's nicer than quietly having two different union implementations or quietly breaking the existing `Option` impl. ### Crate Publish Ordering I've modified the order in which CI publishes crates to ensure that we don't publish a crate without ensuring we already published a crate that it depends upon. ## TODO - [ ] Queue a follow-up `[patch]`-removing PR. |