-
Notifications
You must be signed in to change notification settings - Fork 808
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
Bump SSZ version for larger bitfield SmallVec
#6915
base: unstable
Are you sure you want to change the base?
Conversation
This reverts commit 2d39282.
We have a node running this branch on infra, are you planning to post some charts with metrics once it's been running for a little while? I guess we're looking for lower total allocations per second, and maybe (indirectly) lower memory usage due to fragmentation. I don't imagine we get a noticeable reduction in CPU usage? |
I'm still collecting metrics in an attempt to show this has some effect. I've added some new jemalloc |
Ok, so I have some metrics. The good news is that we can see a drop in allocations associated with this PR. The less good news is that I can't find any other metric that is affected by this change. Below are metrics of ![]() What we can observe here is a reduction in the count of allocations. In particular, these are temporary allocations, as observed by the peaks in allocations and deallocations (I summed these in Grafana and they net-out to a flat line). Notably, I did not see any changes in:
Therefore, we have managed to reduce the allocation count without being able to observe any tangible second-order benefit. Should we still merge this PR, I think yes. Primarily because reducing the allocation count feels like good hygiene to me. However, I am certainly open to opposing arguments. |
Issue Addressed
NA
Proposed Changes
Bumps the
ethereum_ssz
version, along with other crates that share the dep.Primarily, this give us bitfields which can store 128 bytes on the stack before allocating, rather than 32 bytes (sigp/ethereum_ssz#38). The validator count has increase massively since we set it at 32 bytes, so aggregation bitfields (et al) now require a heap allocation. This new value of 128 should get us to ~2m active validators.
Additional Info
ssz_types
toethereum_ssz
, so there's some non-substantial changes to error handling.MetaData
struct inlighthouse_network
holds two bitfields and therefore now has a larger stack size. I had toArc
it to avoid some clippy lints about disparate enum sizes.Attestation
is now 96 bytes larger than it was before (32-128). You'll see some tests updated to expect these new sizes.