fix: reduce hot-path allocation, cloning, and lock contention#524
fix: reduce hot-path allocation, cloning, and lock contention#524varex83agent wants to merge 1 commit into
Conversation
Implements the eight hot-path optimizations from the "hot-path allocation, cloning, and lock-contention reduction" plan. All changes are perf-only and preserve functional equivalence with Charon v1.7.1. - T01 p2p: add alloc-free `PeerStore::has_connection`; migrate the five emptiness-only `connections_to_peer(..).is_empty()` call sites. - T02 qbft: thread one `Arc<QbftConsensusMsg>` across peers on broadcast instead of an N-1 deep clone; move `msg`/`justification`/`values` out of `pb_msg` on receive; `values_by_hash` consumes its input. - T03 eth2api: `Arc`-wrap the cached validator maps, add a read-lock fast path, and release the write lock across the beacon-node fetch (re-checking on re-acquire). - T04 scheduler: replace the O(N) `to_entries()` status-metric scan with a per-pubkey last-status map, zeroing only the previously-reported series. - T05 scheduler: cache `slots_per_epoch` on the actor at build time instead of a beacon-node round-trip on every duty lookup. - T06 parsigex: acquire the peer-store guard once per broadcast and share the encoded payload via `Arc<[u8]>` instead of a per-target `Vec<u8>` copy. - T07 parsigdb: fold the threshold check into `store` under the lock and return only the matched set, dropping the per-store full-accumulator clone. - T08 ssz: pass the buffer by value into `merkleize_impl` (one copy, not two); reuse one `Sha256` in `default_hash_fn`; drop the `put_bitlist` clone. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
varex83
changed the title
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM, some small nits only.
| // Move the validated parts out of `pb_msg` (it is dropped after this), | ||
| // avoiding a clone of the message, justifications, and values. |
There was a problem hiding this comment.
In general, these AI comments do not provide much value. They explain what is immediately happening in the code. Unless the code is not clear then prefer to drop it. The comments will eventually get stale and add confusion later.
| // Wrap once so the fan-out shares a single allocation across all | ||
| // peers rather than deep-cloning the (potentially multi-MB) | ||
| // payload per target, matching Charon's shared-pointer broadcast. |
There was a problem hiding this comment.
Example: here, the fact that we're using an Arc is required due to the BroadcastCommand signature. If you need to justify its usage do it on the type definition itself, or just drop it.
| /// `statusGauge.Reset(pubkey, ...)` + `Set(1)` in O(1) per validator: it zeroes | ||
| /// only the previously-reported series for this pubkey (when the status | ||
| /// changed) and sets the current one to 1. | ||
| fn submit_validator_status_metric(pubkey: &types::PubKey, status: &str) { |
There was a problem hiding this comment.
We should explore if we can make this a feature in Vise itself. We already are referencing it from Git.
| // Fetch the chain constants once at build time (the node is synced at | ||
| // this point) and cache `slots_per_epoch` on the actor, mirroring | ||
| // Charon's memoized-spec behaviour. | ||
| let (_slot_duration, slots_per_epoch) = client.api().fetch_slots_config().await?; |
There was a problem hiding this comment.
We should explore a more general solutions: there are beacon client calls that are expected to not change throughout the lifetime of the application (ex. fetch_slots_config), so we could cache them at the client level.
For now this is fine, but there are multiple places where this function is invoked and we'll end up adding caching code to each one of them.
| let (eth2_cl, pubkeys) = { | ||
| let inner = self.0.read().await; | ||
| (inner.eth2_cl.clone(), inner.pubkeys.clone()) |
There was a problem hiding this comment.
We could refactor this and extract client and pubkeys from inner: these values do not change so no need to put them under the lock.
2 participants
Summary
Implements the eight hot-path optimizations from the hot-path allocation, cloning, and lock-contention reduction plan. Every change is perf-only and preserves functional equivalence with Charon v1.7.1 — no wire format, validation, ordering, or metric semantics change.
Tasks
p2pPeerStore::has_connection; migrate the 5 emptiness-onlyconnections_to_peer(..).is_empty()callers (qbft, frostp2p, bcast, parsigex, monitoringapi).consensus(qbft)Arc<QbftConsensusMsg>across all peers on broadcast instead of an N-1 deep clone of the multi-MB payload; movemsg/justification/valuesout ofpb_msgon receive;values_by_hashnow consumesVec<Any>.eth2apiArc-wrap the cachedActiveValidators/CompleteValidatorsmaps (cheap clones viaDeref), add a read-lock fast path on a warm cache, and release the write lock across the beacon-node fetch (re-checking on re-acquire).core(scheduler)to_entries()status-metric scan with a per-pubkey last-status map, zeroing only the previously-reported series — mirrors Charon'sstatusGauge.Reset.core(scheduler)slots_per_epochon the actor at build time instead of a beacon-node round-trip on every duty lookup.parsigexhas_connection) and share the encoded payload viaArc<[u8]>instead of a per-targetVec<u8>copy.core(parsigdb)storeunder the lock and return only the matched set via aStoreOutcomeenum, dropping the per-store full-accumulator deep clone.sszmerkleize_impl(one copy instead of two); reuse a singleSha256indefault_hash_fn; drop theput_bitlistscratch clone.Tests
New unit/regression tests added for each task (peer-store connectivity,
Arc::ptr_eqshared-buffer assertions for qbft and parsigex, concurrent valcache misses, per-pubkey status-metric transitions, cached-slots_per_epochepoch math, parsigdb duplicate/mismatch paths, and ssz golden merkleization roots).Quality gates
cargo +nightly fmt --all --check✅cargo clippy --workspace --all-targets --all-features -- -D warnings✅cargo test --workspace --all-features✅ (all suites pass)cargo deny check✅🤖 Generated with Claude Code