Introduce InstalledSchedulerPool trait #33934
Conversation
| /// Schedules, executes, and commits transactions under encapsulated implementation | ||
| /// | ||
| /// The following chart illustrates the ownership/reference interaction between inter-dependent | ||
| /// objects across crates: | ||
| /// | ||
| /// ```mermaid | ||
| /// graph TD | ||
| /// Bank["Arc#lt;Bank#gt;"] | ||
| /// | ||
| /// subgraph solana-runtime | ||
| /// BankForks; | ||
| /// BankWithScheduler; | ||
| /// Bank; | ||
| /// LoadExecuteAndCommitTransactions(["load_execute_and_commit_transactions()"]); | ||
| /// SchedulingContext; | ||
| /// InstalledSchedulerPool{{InstalledSchedulerPool}}; | ||
| /// InstalledScheduler{{InstalledScheduler}}; | ||
| /// end | ||
| /// | ||
| /// subgraph solana-unified-scheduler-pool | ||
| /// SchedulerPool; | ||
| /// PooledScheduler; | ||
| /// ScheduleExecution(["schedule_execution()"]); | ||
| /// end | ||
| /// | ||
| /// subgraph solana-ledger | ||
| /// ExecuteBatch(["execute_batch()"]); | ||
| /// end | ||
| /// | ||
| /// ScheduleExecution -. calls .-> ExecuteBatch; | ||
| /// BankWithScheduler -. dyn-calls .-> ScheduleExecution; | ||
| /// ExecuteBatch -. calls .-> LoadExecuteAndCommitTransactions; | ||
| /// linkStyle 0,1,2 stroke:gray,color:gray; | ||
| /// | ||
| /// BankForks -- owns --> BankWithScheduler; | ||
| /// BankForks -- owns --> InstalledSchedulerPool; | ||
| /// BankWithScheduler -- refs --> Bank; | ||
| /// BankWithScheduler -- owns --> InstalledScheduler; | ||
| /// SchedulingContext -- refs --> Bank; | ||
| /// InstalledScheduler -- owns --> SchedulingContext; | ||
| /// | ||
| /// SchedulerPool -- owns --> PooledScheduler; | ||
| /// SchedulerPool -. impls .-> InstalledSchedulerPool; | ||
| /// PooledScheduler -. impls .-> InstalledScheduler; | ||
| /// PooledScheduler -- refs --> SchedulerPool; | ||
| /// ``` |
There was a problem hiding this comment.
fyi, this is rendered like this:
| //! Currently, there are only two things: minimal InstalledScheduler trait and an auxiliary type | ||
| //! called BankWithScheduler.. This file will be populated by later PRs to align with the filename. |
ryoqun
changed the title
Codecov Report
@@ Coverage Diff @@
## master #33934 +/- ##
=========================================
- Coverage 81.9% 81.9% -0.1%
=========================================
Files 809 809
Lines 218241 218277 +36
=========================================
+ Hits 178794 178806 +12
- Misses 39447 39471 +24 |
| @@ -27,6 +100,9 @@ use {mockall::automock, qualifier_attr::qualifiers}; | |||
| allow(unused_attributes, clippy::needless_lifetimes) | |||
| )] | |||
| pub trait InstalledScheduler: Send + Sync + Debug + 'static { | |||
| fn id(&self) -> SchedulerId; | |||
| fn pool(&self) -> InstalledSchedulerPoolArc; | |||
There was a problem hiding this comment.
Do we ever need/use the pool for anything other than a call to return_to_pool?
If that's the case, why not just have a direct function on this trait that consumes the appropriate wrapped type (Box<Self>, I think)?
| @@ -518,10 +513,11 @@ mod tests { | |||
| fn test_scheduler_pause() { | |||
| solana_logger::setup(); | |||
|
|
|||
| let bank = Arc::new(crate::bank::tests::create_simple_test_bank(42)); | |||
| let bank = &Arc::new(crate::bank::tests::create_simple_test_bank(42)); | |||
There was a problem hiding this comment.
why all these changes to use an &Arc<..> instead of owned Arc?
There was a problem hiding this comment.
no strong reason other than to minimize diff. ;)
There was a problem hiding this comment.
well, thanks for pointing out. i've found way of further reducing the diff: 5df66e0
|
I'll merge this, although the ci is running: the last commit is just about |
Problem
InstalledScheduleris expected to be backed by multiple threads. thus, it's desirable to be reused to avoid thread recreation cost.Summary of Changes
introduce some pooling mechanism.
also, finished off all the doc comments.
note that this pr contains only the trait interface bits. the actual impl will be added by later pr.
extracted from: #33070