Skip to content

Scheduler Optimization: Pre-allocate vectors during buffering checks#1725

Merged
apfitzge merged 3 commits intoanza-xyz:masterfrom
apfitzge:drain_optimize
Jun 26, 2024
Merged

Scheduler Optimization: Pre-allocate vectors during buffering checks#1725
apfitzge merged 3 commits intoanza-xyz:masterfrom
apfitzge:drain_optimize

Conversation

@apfitzge
Copy link
Copy Markdown

Problem

  • No need to allocate the each chunk

Summary of Changes

  • Pre-allocate buffers for chunk processing.
  • Use drain to avoid lifetime issues with the vecs continuing to live on

Pulled out of #1663 (thanks @t-nelson)

Fixes #

@apfitzge apfitzge marked this pull request as ready for review June 13, 2024 11:25
@apfitzge apfitzge requested a review from t-nelson June 13, 2024 11:25
@apfitzge apfitzge requested review from ryoqun and tao-stones June 20, 2024 11:55
@@ -531,9 +531,9 @@ impl SchedulerController {
let mut num_dropped_on_capacity: usize = 0;
let mut num_buffered: usize = 0;
for (((packet, transaction), fee_budget_limits), _) in arc_packets
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename _ => _check_result.

Comment on lines +484 to +486
let mut arc_packets = Vec::with_capacity(CHUNK_SIZE);
let mut transactions = Vec::with_capacity(CHUNK_SIZE);
let mut fee_budget_limits_vec = Vec::with_capacity(CHUNK_SIZE);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: using smallvec further reduces allocation entirely per each buffer_packets() invocation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL smallvec

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for arrayvec, since it fully guarantees no heap-allocation. I think it's also slightly faster because there's no need to check for overflow onto the heap when we drain them.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryoqun lmk if you think ArrayVec is good here 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, by principle, it'll be slightly faster.

unlike smallvec, arrayvec will panic!() if we're about to .push() more than CHUNK_SIZE. However, that seems to be guaranteed not to happen.

Copy link
Copy Markdown
Member

@ryoqun ryoqun Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the trio of these arrayvec will consume ~36 kb of stack in total, according to rust-analyzer:

// size = 1032 (0x408), align = 0x8
let mut arc_packets: ArrayVec<Arc<ImmutableDeserializedPacket>, 128>
// size = 31752 (0x7C08), align = 0x8
let mut transactions: ArrayVec<SanitizedTransaction, 128>
// size = 4104 (0x1008), align = 0x8
let mut fee_budget_limits_vec: ArrayVec<FeeBudgetLimits, 128>

That again will be okay, considering linux's default thread stack size. (8 MiB)

ryoqun
ryoqun previously approved these changes Jun 21, 2024
Copy link
Copy Markdown
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with nit

tao-stones
tao-stones previously approved these changes Jun 21, 2024
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@apfitzge apfitzge dismissed stale reviews from tao-stones and ryoqun via b13da62 June 24, 2024 17:08
@apfitzge apfitzge requested a review from ryoqun June 25, 2024 15:37
@@ -479,14 +480,14 @@ impl SchedulerController {

const CHUNK_SIZE: usize = 128;
let lock_results: [_; CHUNK_SIZE] = core::array::from_fn(|_| Ok(()));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(off topic) this can be rewritten with inline const once we update to rust 1.79.

Copy link
Copy Markdown
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@apfitzge apfitzge merged commit 9ee36fa into anza-xyz:master Jun 26, 2024
@apfitzge apfitzge deleted the drain_optimize branch June 26, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants