perf: anchor is mostly None#631
Conversation
WalkthroughBallot fields were made private and Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/amaru-kernel/src/ballot.rs (1)
66-76: Minor: Consider using the constructor in decode for consistency.The decode implementation directly constructs the struct bypassing
Ballot::new(). While this works fine (the decoder returnsOption<Box<Anchor>>directly), using the constructor would be more consistent with the encapsulation pattern.♻️ Optional refactor to use constructor
impl<'d, C> cbor::decode::Decode<'d, C> for Ballot { fn decode(d: &mut cbor::Decoder<'d>, ctx: &mut C) -> Result<Self, cbor::decode::Error> { heterogeneous_array(d, |d, assert_len| { assert_len(2)?; - Ok(Self { - vote: d.decode_with(ctx)?, - anchor: d.decode_with(ctx)?, - }) + Ok(Ballot::new( + d.decode_with(ctx)?, + d.decode_with(ctx)?, + )) }) } }This way, if you ever change the internal representation again, the decode logic benefits from the constructor's transformation automatically. That said, the current approach might be more efficient since the decoder already produces
Option<Box<Anchor>>, so no worries either way!
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/amaru-kernel/src/ballot.rscrates/amaru-ledger/src/bootstrap.rscrates/amaru-ledger/src/context/default/validation.rscrates/amaru-ledger/src/governance/ratification.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-ledger/src/context/default/validation.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-ledger/src/context/default/validation.rscrates/amaru-ledger/src/governance/ratification.rscrates/amaru-kernel/src/ballot.rscrates/amaru-ledger/src/bootstrap.rs
📚 Learning: 2025-08-19T09:54:04.412Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-ledger/src/summary/stake_distribution.rs:219-225
Timestamp: 2025-08-19T09:54:04.412Z
Learning: In stake distribution calculations for governance voting, pool deposit refunds are intentionally handled asymmetrically: DRep voting stake includes the refund (`dreps_voting_stake += account.lovelace + drep_deposits + refund`) while Pool voting stake does not include the refund (`let delta = account.lovelace + pool_deposits`). This behavior matches the Haskell Cardano ledger reference implementation and is by design, not a bug.
Applied to files:
crates/amaru-ledger/src/governance/ratification.rs
🧬 Code graph analysis (3)
crates/amaru-ledger/src/context/default/validation.rs (1)
crates/amaru-kernel/src/ballot.rs (3)
new(37-42)vote(44-46)anchor(48-50)
crates/amaru-kernel/src/ballot.rs (1)
crates/amaru-ledger/src/context/default/validation.rs (2)
new(47-58)vote(228-236)
crates/amaru-ledger/src/bootstrap.rs (3)
crates/amaru-kernel/src/ballot.rs (2)
new(37-42)vote(44-46)crates/amaru-ledger/src/context/assert.rs (1)
vote(266-267)crates/amaru-ledger/src/context.rs (1)
vote(251-251)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build x86_64/windows
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
crates/amaru-ledger/src/governance/ratification.rs (1)
532-546: Crikey, mate! Clean accessor migration across all voter types.The update from direct field access to
ballot.vote()is bang-on consistent across all five voter variants. The returned&Votereferences are properly borrowed from the input map, so no lifetime dramas here. Nice one! 🎯crates/amaru-ledger/src/bootstrap.rs (1)
1056-1078: G'day! Solid constructor usage throughout the import path.All three ballot instantiations in
import_votesconsistently useBallot::new(vote, None)instead of the struct literal. These are all constructing ballots without anchors (the common case), which is exactly where this optimization shines — no heap allocation, just that sweet 16-byte footprint. Beaut! 🦘crates/amaru-ledger/src/context/default/validation.rs (1)
228-236: Strewth! Constructor pattern applied perfectly in the validation context.The
ProposalsSlice::voteimplementation now usesBallot::new(vote, anchor), letting the constructor handle the boxing logic internally when the anchor isSome. Dead simple migration, no worries here! 🎬crates/amaru-kernel/src/ballot.rs (4)
18-34: Bloody brilliant memory optimization, mate!The field privatization and boxing of
anchoris spot-on for this use case. Your documentation nails why this works:
- Anchor is None ~60% of the time on mainnet (way more on preprod)
- Struct shrinks from 64B to 16B when anchor is None
- The heap indirection cost for Some cases is negligible since anchors are rarely accessed
The Box keeps the on-chain data available for users even though the ledger doesn't touch it. Classic space-time trade-off done right! 🎮
36-51: Constructor and accessors looking mint!The API design is clean as:
new()handles the boxing via.map(Box::new)— smooth operatorvote()returns&Vote— straightforwardanchor()uses.as_deref()to unwrap the Box back toOption<&Anchor>— proper Rust wizardryThis encapsulation lets you change the internal representation without breaking the public API. Choice! 🍿
53-64: CBOR encode adapted perfectly to the new accessors.Using
self.vote()andself.anchor()keeps the encode implementation clean and consistent with the encapsulation. The wire format stays the same, so no breaking changes for serialization. Noice! 🎯
87-94: Test updated to match the new constructor API — champion!The proptest generator now uses
Ballot::new(vote, anchor)instead of the struct literal. Keeps the tests aligned with the public API, which is exactly how it should be. Top shelf! 🏆
2 participants
On preprod the anchor of a Ballot is <1% of the time
Someon mainnet it is more like 42%. In this PR we change frompub anchor: Option<Anchor>topub anchor: Option<Box<Anchor>>. Witch makes Ballot go from 64 bytes to 16, but at the cost of adding an additional allocation for theBoxwhen the anchor is Some. For <1%Somethat is a clear win. For the mainnet numbers we have approximately(1497*64)=95808vs(1497*16 + 636 * 56)=59568.This is a less drastic alternative to #601
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.