ledger: Build the grouped slot leaders manually #8451

vadorovsky · 2025-10-13T15:04:21Z

Problem

Building them with itertools::into_group_map takes 20.9ms.

Summary of Changes

Building them manually with a pre-allocated hash map, using pubkey hasher, takes 5.2ms.

Ref: #8280

codecov-commenter · 2025-10-13T15:56:18Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.1%. Comparing base (d8dcdad) to head (80b2e04).
⚠️ Report is 16 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #8451     +/-   ##
=========================================
- Coverage    83.2%    83.1%   -0.1%     
=========================================
  Files         846      846             
  Lines      368550   368566     +16     
=========================================
- Hits       306656   306536    -120     
- Misses      61894    62030    +136

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

HaoranYi · 2025-10-14T14:13:33Z

ledger/src/leader_schedule/identity_keyed.rs

    slot_leaders: Vec<Pubkey>,
    // Inverted index from pubkeys to indices where they are the leader.
-    leader_slots_map: HashMap<Pubkey, Arc<Vec<usize>>>,
+    leader_slots_map: HashMap<Pubkey, Vec<usize>, PubkeyHasherBuilder>,


We might need to change this fn to not cloning the vec anymore since it is no longer an Arc.

Thanks for pointing this!

I tried to remove this cloned() and make it work with Vec<usize> (without Arc), but the problem is that the returned iterator is used outside of the LeaderSchedule, so then compiler was rightfully complaining about lifetimes and returning references to local values.

So I decided to put the Arc back. Now I can see its purpose.

BTW, adding Arc back unfortunately increases the execution time from 5.2ms to 9.9ms. 🥲 But I have no better ideas and that's still better than cloning.

looks reasonable to me.

I find a way to remove Arc.

#8499

The Arc was needed because get_leader_upcoming_slots returned a boxed iterator requiring ownership of the Vec. By modifying get_leader_upcoming_slots to return a borrowed iterator (Box<dyn Iterator + '_>), and collecting the Arc 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.

HaoranYi · 2025-10-14T14:23:47Z

Excellent work! I have just one comment!

HaoranYi

LGTM. Please wait for other reviewer's approval before merge.

brooksprumo · 2025-10-15T16:14:30Z

I'm going to bow out and defer to the other reviewers since I'm OOO.

jstarry · 2025-10-16T02:06:12Z

We should probably review our uses of itertools across the codebase because many of the utility methods create hashsets and hashmaps which are not pre-allocated.

vadorovsky · 2025-10-16T07:58:00Z

We should probably review our uses of itertools across the codebase because many of the utility methods create hashsets and hashmaps which are not pre-allocated.

I'm yet to profile all uses of into_group_map() in ledger/runtime/accounts-db, but my gut feeling for now is that we should move away from it entirely, especially given that the manual alternative I'm replacing it with is basically this few-liner:

let mut grouped = HashMap::with_capacity_and_hasher(cap, hasher);
for (key, value) in input {
    grouped_slot_leaders
        .entry(value)
        .and_modify(|keys| {
            keys.push(key);
    })
    .or_insert(vec![key]);
}

Perhaps I should already move it to some common function in solana-perf.

Given the lack of elasticity of itertools (no way of providing capacities and hashers), I think its usage is a footgun. I tried to make it accept custom hashers, but the PR got closed: rust-itertools/itertools#1057.

kskalski · 2025-10-16T10:56:37Z

ledger/src/leader_schedule/identity_keyed.rs

+                    let slots = Arc::get_mut(slots).expect("should be the only reference");
+                    slots.push(slot)
+                })
+                .or_insert(Arc::new(vec![slot]));


could also experiment with smallvec, with just a slot value you could keep like 2-3 elements inline without allocating

Good idea. Or maybe even arrayvec.

in the profile, I think you see grow taking a lot of time, so if anything you should allocate with higher capacity?

Oh it's Vec<usize>, yeah use arrayvec

jstarry · 2025-10-16T11:37:10Z

ledger/src/leader_schedule/identity_keyed.rs

+            }
+            None => HashMap::with_hasher(PubkeyHasherBuilder::default()),
+        };
+        for (slot, leader) in slot_leaders.iter().enumerate() {


Kamil's comment made me realize that slot_leaders will always be setup so that the same leader will always the same for 4 consecutive slots. We could use that fact to do more optimizing

HaoranYi · 2025-10-16T16:22:57Z

We should probably review our uses of itertools across the codebase because many of the utility methods create hashsets and hashmaps which are not pre-allocated.

Good point — applying the same pattern to staked_nodes in stake_cache gave us a 3–4× speed-up as well: #8516

Building them with `itertools::into_group_map` takes 20.9ms. Building them manually with a pre-allocated hash map, using pubkey hasher, takes 5.2ms.

vadorovsky force-pushed the compute-epoch-schedule-group-map branch 2 times, most recently from e42e215 to cb8fedb Compare October 13, 2025 15:14

vadorovsky force-pushed the compute-epoch-schedule-group-map branch 2 times, most recently from 37b32a9 to b758fe5 Compare October 14, 2025 07:07

vadorovsky marked this pull request as ready for review October 14, 2025 07:24

vadorovsky requested review from HaoranYi and brooksprumo October 14, 2025 07:24

HaoranYi reviewed Oct 14, 2025

View reviewed changes

HaoranYi previously approved these changes Oct 15, 2025

View reviewed changes

vadorovsky requested review from alessandrod and jstarry October 15, 2025 15:08

brooksprumo removed their request for review October 15, 2025 16:14

jstarry previously approved these changes Oct 16, 2025

View reviewed changes

kskalski reviewed Oct 16, 2025

View reviewed changes

jstarry reviewed Oct 16, 2025

View reviewed changes

vadorovsky mentioned this pull request Oct 17, 2025

Remove Arc from leader_slots_map in LeaderSchedule #8499

Merged

ledger: Build the grouped slot leaders manually

80b2e04

Building them with `itertools::into_group_map` takes 20.9ms. Building them manually with a pre-allocated hash map, using pubkey hasher, takes 5.2ms.

vadorovsky dismissed stale reviews from jstarry and HaoranYi via 80b2e04 October 21, 2025 08:49

vadorovsky force-pushed the compute-epoch-schedule-group-map branch from cd7b5b6 to 80b2e04 Compare October 21, 2025 08:49

ledger: Build the grouped slot leaders manually #8451

Are you sure you want to change the base?

ledger: Build the grouped slot leaders manually #8451

Uh oh!

Conversation

vadorovsky commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov-commenter commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadorovsky Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadorovsky Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoranYi commented Oct 14, 2025

Uh oh!

HaoranYi left a comment

Choose a reason for hiding this comment

Uh oh!

brooksprumo commented Oct 15, 2025

Uh oh!

jstarry commented Oct 16, 2025

Uh oh!

vadorovsky commented Oct 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoranYi commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vadorovsky commented Oct 13, 2025 •

edited

Loading

codecov-commenter commented Oct 13, 2025 •

edited

Loading

vadorovsky Oct 15, 2025 •

edited

Loading

vadorovsky Oct 15, 2025 •

edited

Loading