Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

TransactionScheduler: Clean already processed or old transactions from container #34233

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

apfitzge
Copy link
Contributor

Problem

  • There's a lot of already processed or old transactions that make it into the buffer
  • They aren't cleaned away before scheduling

Summary of Changes

  • Remove these transactions out of the buffer before leader slot comes

Blocked by #34232

Fixes #

@apfitzge apfitzge force-pushed the scheduler_clear_old branch from 0c61b4c to fb2acf3 Compare November 28, 2023 17:15
@apfitzge apfitzge marked this pull request as ready for review November 28, 2023 17:41
@apfitzge apfitzge requested a review from tao-stones November 28, 2023 17:41
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Merging #34233 (fb2acf3) into master (b741c9c) will increase coverage by 0.0%.
Report is 7 commits behind head on master.
The diff coverage is 65.9%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34233   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         819      819           
  Lines      219387   219448   +61     
=======================================
+ Hits       179690   179785   +95     
+ Misses      39697    39663   -34     

Copy link
Contributor

@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 - believe thread local MI scheduler also frequently checks and drops expired/processed tx out of queue. Just a small question regarding how frequently it could be performed. The question does not need to block the PR's merge.

let (_, clean_time_us) = measure_us!(self.clean_queue());
saturating_add_assign!(self.timing_metrics.clean_time_us, clean_time_us);
}
BufferedPacketsDecision::Hold => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to drop expired or processed txs when getting close to becoming leader. Assume doing so only when ForwardAndHold but not Hold is due to performance concerns? Depends on clean_time_us, maybe can do queue cleaning more frequently to avoid scheduler spending too much time on cleaning instead of receiving when close to be leader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so Hold can happen for a few reasons, one of which is it's our leader slot but the bank isn't ready yet. In that case, you'd expect to get the bank really soon, so we don't really want to do anything until the bank is there and we can schedule.

Previously, the TLMI (thread-local multi-iterator) would filter during ForwardAndHold before forwarding. So this PR is kind of re-introducing that behavior, but just not forwarding.

const MAX_TRANSACTION_CHECKS: usize = 10_000;
let mut transaction_ids = Vec::with_capacity(MAX_TRANSACTION_CHECKS);

while let Some(id) = self.container.pop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, could continer.size > MAX_TRANSACTION_CHECKS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it could be. My intent here was to keep the runtime of this fn very short by only cleaning the top-of-queue, i.e. only trnasactions we're very likely to schedule early on during leader slot.

@apfitzge apfitzge merged commit df88937 into solana-labs:master Nov 29, 2023
@apfitzge apfitzge deleted the scheduler_clear_old branch November 29, 2023 00:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants