Remove Arc from leader_slots_map in LeaderSchedule #8499
Conversation
HaoranYi
force-pushed
the
remove-arc-from-leader-slots-map
branch
from
4e4163a to
2b1a259
Compare
Changed `HashMap<Pubkey, Arc<Vec<usize>>>` to `HashMap<Pubkey, Vec<usize>>` in the leader schedule implementation. The Arc was previously needed because `get_leader_upcoming_slots` returned a boxed iterator requiring ownership of the Vec. By modifying the method to return a borrowed iterator (`Box<dyn Iterator + '_>`), and collecting the Arc<LeaderSchedule> references upfront in `next_leader_slot`, the schedules stay alive for the entire iterator chain, eliminating the need for Arc wrapping and avoiding intermediate Vec allocations. Changes: - Updated trait method signature and implementations - Simplified `invert_slot_leaders` to return HashMap without Arc wrapping - Modified `get_leader_upcoming_slots` to return borrowed iterator - Collect leader schedules in `next_leader_slot` before creating iterators - Removed Arc imports from identity_keyed and vote_keyed modules
HaoranYi
force-pushed
the
remove-arc-from-leader-slots-map
branch
from
2b1a259 to
4f61bd4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8499 +/- ##
========================================
Coverage 83.1% 83.1%
========================================
Files 848 848
Lines 368309 368314 +5
========================================
+ Hits 306273 306431 +158
+ Misses 62036 61883 -153 🚀 New features to boost your workflow:
|
| .map(|epoch| self.get_epoch_schedule_else_compute(epoch, bank)) | ||
| .while_some() | ||
| .zip(epoch..) | ||
| .collect(); |
There was a problem hiding this comment.
Did you have a chance to measure how much does this collect take? This change looks like a trade-off - slower next_leader_slot (the question is - how much) for faster epoch boundary (~5ms).
I can profile it, but I'm AFK now, so I could do it tomorrow.
There was a problem hiding this comment.
It will be small. There is going to be at most two Arc of leader schedules.
There was a problem hiding this comment.
You're right, it's like 0.1ms per each replay.
|
pls merge whenever you can, so I can put better profiles in #8451 🙂 |
Yeah, it’s merged. Brooks is on vacation, so let’s skip his approval for now — if he has any feedback later, we can handle it in a follow-up PR. |
4 participants
Problem
The
leader_slots_mapinLeaderScheduleusesHashMap<Pubkey, Arc<Vec<usize>>>, but theArcwrapping is unnecessary overhead.The Arc was previously needed because
get_leader_upcoming_slotsreturned a boxed iterator requiring ownership of the Vec. This forced cloning of the Arc on every call.Summary of Changes
Changed
HashMap<Pubkey, Arc<Vec<usize>>>toHashMap<Pubkey, Vec<usize>>in the leader schedule implementation.By modifying
get_leader_upcoming_slotsto return a borrowed iterator (Box<dyn Iterator + '_>), and collecting theArc<LeaderSchedule>references upfront innext_leader_slot, the schedules stay alive for the entire iterator chain, eliminating the need for Arc wrapping and avoiding intermediate Vec allocations.Changes:
invert_slot_leadersto return HashMap without Arc wrappingget_leader_upcoming_slotsto return borrowed iteratornext_leader_slotbefore creating iteratorsFixes #