Add BankWithScheduler for upcoming scheduler code #33704
Conversation
ryoqun
force-pushed
the
bank-with-scheduler
branch
from
cb7a6c4 to
1de3ef6
Compare
| bank.check_program_modification_slot = | ||
| self.root.load(Ordering::Relaxed) < self.highest_slot_at_startup; | ||
|
|
||
| let bank = Arc::new(bank); | ||
| let prev = self.banks.insert(bank.slot(), bank.clone()); | ||
| let bank = BankWithScheduler::new_without_scheduler(Arc::new(bank)); |
There was a problem hiding this comment.
this is the sole call site of BankWithScheduler constructor, and it is always creating with no scheduler for now.
There was a problem hiding this comment.
It looks like we call it above for the insertion of initial forks as well? in new_from_banks
Might just be misunderstanding; this is the only call site where we plan to construct with a scheduler in the future, right?
There was a problem hiding this comment.
this is the only call site where we plan to construct with a scheduler (emphasis mine) in the future, right?
yeah. new_from_banks shouldn't have scheduler because they should be frozen, iirc.
Codecov Report
@@ Coverage Diff @@
## master #33704 +/- ##
=======================================
Coverage 81.8% 81.8%
=======================================
Files 806 807 +1
Lines 217428 217492 +64
=======================================
+ Hits 178058 178122 +64
Misses 39370 39370 |
ryoqun
force-pushed
the
bank-with-scheduler
branch
from
1de3ef6 to
5c2e0d4
Compare
There was a problem hiding this comment.
filename is completely misnomer now... ;)
There was a problem hiding this comment.
Left a couple of comments, but going to take another pass on this later.
The only thing I didn't make a separate comment on is naming...which I think we discussed before. Still seems weird to have a BankWithScheduler::new_without_scheduler haha, but I'm not sure I can come up with a better name
runtime/src/bank_forks.rs
Outdated
| @@ -82,7 +83,10 @@ impl BankForks { | |||
| } | |||
|
|
|||
| pub fn banks(&self) -> HashMap<Slot, Arc<Bank>> { | |||
There was a problem hiding this comment.
Fine to leave this as a follow-up task, but I don't think we need this interface to be entirely unchanging.
None of the call-sites care that it's specifically an Arc<Bank>, and actually most don't even require ownership of the HashMap itself!
I checked and this compiles just fine:
pub fn banks(&self) -> &HashMap<Slot, BankWithScheduler> {
&self.banks
}There was a problem hiding this comment.
Not quite as simple for frozen_banks, but that one may also be better off returning an iterator, since most use-cases seem to just be iterating rather than requireing ownership of the map.
There was a problem hiding this comment.
I don't think we need this interface to be entirely unchanging.
good catch: 0aed626
Not quite as simple for frozen_banks, but that one may also be better off returning an iterator, since most use-cases seem to just be iterating rather than requireing ownership of the map.
yeah. you're right. however, I'll pass to cleaning this...
| bank.check_program_modification_slot = | ||
| self.root.load(Ordering::Relaxed) < self.highest_slot_at_startup; | ||
|
|
||
| let bank = Arc::new(bank); | ||
| let prev = self.banks.insert(bank.slot(), bank.clone()); | ||
| let bank = BankWithScheduler::new_without_scheduler(Arc::new(bank)); |
There was a problem hiding this comment.
It looks like we call it above for the insertion of initial forks as well? in new_from_banks
Might just be misunderstanding; this is the only call site where we plan to construct with a scheduler in the future, right?
thanks for careful reviews. and sorry about skipping some suggested cleanup. i have to move on... |
ryoqun
force-pushed
the
bank-with-scheduler
branch
from
0aed626 to
8758ea9
Compare
Problem
There's no nice mechanism to accompany a scheduler with a bank for both replaying and banking stage usages in a uniform fashion.
Summary of Changes
Introduce it (
BankWithScheduler) before actual introduction of scheduler code...the code change is rather broad. but i tried to make this pr straightforward as much as possible..
extracted from: #33070