Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

set_bank takes owned Arc<Bank> #31717

Merged
merged 1 commit into from
May 23, 2023
Merged

set_bank takes owned Arc<Bank> #31717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion banking-bench/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ fn main() {
);

assert!(poh_recorder.read().unwrap().bank().is_none());
poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);
assert!(poh_recorder.read().unwrap().bank().is_some());
debug!(
"new_bank_time: {}us insert_time: {}us poh_time: {}us",
Expand Down
2 changes: 1 addition & 1 deletion core/src/banking_stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,7 @@ mod tests {

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);
let pubkey = solana_sdk::pubkey::new_rand();
let keypair2 = Keypair::new();
let pubkey2 = solana_sdk::pubkey::new_rand();
Expand Down
20 changes: 10 additions & 10 deletions core/src/banking_stage/consumer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,7 +740,7 @@ mod tests {
let recorder = poh_recorder.new_recorder();
let poh_recorder = Arc::new(RwLock::new(poh_recorder));

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

Expand Down Expand Up @@ -899,7 +899,7 @@ mod tests {

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);
let (replay_vote_sender, _replay_vote_receiver) = unbounded();
let committer = Committer::new(
None,
Expand Down Expand Up @@ -1026,7 +1026,7 @@ mod tests {

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);
let (replay_vote_sender, _replay_vote_receiver) = unbounded();
let committer = Committer::new(
None,
Expand Down Expand Up @@ -1098,7 +1098,7 @@ mod tests {

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);
let (replay_vote_sender, _replay_vote_receiver) = unbounded();
let committer = Committer::new(
None,
Expand Down Expand Up @@ -1217,7 +1217,7 @@ mod tests {
let recorder = poh_recorder.new_recorder();
let poh_recorder = Arc::new(RwLock::new(poh_recorder));

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

Expand Down Expand Up @@ -1517,7 +1517,7 @@ mod tests {

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);

let shreds = entries_to_test_shreds(
&entries,
Expand Down Expand Up @@ -1655,7 +1655,7 @@ mod tests {

let poh_simulator = simulate_poh(record_receiver, &poh_recorder);

poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);

let shreds = entries_to_test_shreds(
&entries,
Expand Down Expand Up @@ -1754,7 +1754,7 @@ mod tests {
assert_eq!(buffered_packet_batches.len(), num_conflicting_transactions);
// When the working bank in poh_recorder is Some, all packets should be processed.
// Multi-Iterator will process them 1-by-1 if all txs are conflicting.
poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank, false);
let bank_start = poh_recorder.read().unwrap().bank_start().unwrap();
let banking_stage_stats = BankingStageStats::default();
consumer.consume_buffered_packets(
Expand Down Expand Up @@ -1832,7 +1832,7 @@ mod tests {
assert_eq!(buffered_packet_batches.len(), num_conflicting_transactions);
// When the working bank in poh_recorder is Some, all packets should be processed.
// Multi-Iterator will process them 1-by-1 if all txs are conflicting.
poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank, false);
let bank_start = poh_recorder.read().unwrap().bank_start().unwrap();
consumer.consume_buffered_packets(
&bank_start,
Expand Down Expand Up @@ -1885,7 +1885,7 @@ mod tests {
// When the working bank in poh_recorder is Some, all packets should be processed
// except except for retryable errors. Manually take the lock of a transaction to
// simulate another thread processing a transaction with that lock.
poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank.clone(), false);
let bank_start = poh_recorder.read().unwrap().bank_start().unwrap();

let lock_account = transactions[0].message.account_keys[1];
Expand Down
2 changes: 1 addition & 1 deletion core/src/replay_stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1948,7 +1948,7 @@ impl ReplayStage {
poh_recorder
.write()
.unwrap()
.set_bank(&tpu_bank, track_transaction_indexes);
.set_bank(tpu_bank, track_transaction_indexes);
Copy link
Member

Choose a reason for hiding this comment

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

yay, one less clone for block generation code path. :)

} else {
error!("{} No next leader found", my_pubkey);
}
Expand Down
40 changes: 20 additions & 20 deletions poh/src/poh_recorder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,14 +573,14 @@ impl PohRecorder {
self.leader_last_tick_height = leader_last_tick_height;
}

pub fn set_bank(&mut self, bank: &Arc<Bank>, track_transaction_indexes: bool) {
pub fn set_bank(&mut self, bank: Arc<Bank>, track_transaction_indexes: bool) {
assert!(self.working_bank.is_none());
self.leader_bank_notifier.set_in_progress(bank);
self.leader_bank_notifier.set_in_progress(&bank);
Copy link
Member

Choose a reason for hiding this comment

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

hehe, set_in_progress seems to be a rare exception for this Arc<_> arg fixup saga... lol

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, I think the exception is justified as it does not take ownership of the Arc. It uses the arc to create a weak-reference.

(I know you know this, just making it clear for anyone else looking at the PR why this should be an exception)

let working_bank = WorkingBank {
bank: bank.clone(),
start: Arc::new(Instant::now()),
min_tick_height: bank.tick_height(),
max_tick_height: bank.max_tick_height(),
bank,
start: Arc::new(Instant::now()),
Comment on lines +582 to +583
Copy link
Member

@ryoqun ryoqun May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oh, unfortunate reordering due to unhappy borrow checker. ;)

transaction_index: track_transaction_indexes.then_some(0),
};
trace!("new working bank");
Expand All @@ -595,7 +595,7 @@ impl PohRecorder {
"resetting poh due to hashes per tick change detected at {}",
working_bank.bank.slot()
);
self.reset_poh(working_bank.clone().bank, false);
self.reset_poh(working_bank.bank.clone(), false);
}
}
self.working_bank = Some(working_bank);
Expand Down Expand Up @@ -1057,7 +1057,7 @@ pub fn create_test_recorder(
&poh_config,
exit.clone(),
);
poh_recorder.set_bank(bank, false);
poh_recorder.set_bank(bank.clone(), false);

let poh_recorder = Arc::new(RwLock::new(poh_recorder));
let poh_service = PohService::new(
Expand Down Expand Up @@ -1195,7 +1195,7 @@ mod tests {
Arc::new(AtomicBool::default()),
);

poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank, false);
assert!(poh_recorder.working_bank.is_some());
poh_recorder.clear_bank();
assert!(poh_recorder.working_bank.is_none());
Expand Down Expand Up @@ -1229,7 +1229,7 @@ mod tests {
let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));

// Set a working bank
poh_recorder.set_bank(&bank1, false);
poh_recorder.set_bank(bank1.clone(), false);

// Tick until poh_recorder.tick_height == working bank's min_tick_height
let num_new_ticks = bank1.tick_height() - poh_recorder.tick_height();
Expand Down Expand Up @@ -1298,7 +1298,7 @@ mod tests {
);
assert_eq!(poh_recorder.tick_height, bank.max_tick_height() + 1);

poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);
poh_recorder.tick();

assert_eq!(poh_recorder.tick_height, bank.max_tick_height() + 2);
Expand Down Expand Up @@ -1339,7 +1339,7 @@ mod tests {

bank0.fill_bank_with_ticks_for_tests();
let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
poh_recorder.set_bank(&bank1, false);
poh_recorder.set_bank(bank1.clone(), false);
// Let poh_recorder tick up to bank1.tick_height() - 1
for _ in 0..bank1.tick_height() - 1 {
poh_recorder.tick()
Expand Down Expand Up @@ -1380,7 +1380,7 @@ mod tests {
Arc::new(AtomicBool::default()),
);

poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);
let tx = test_tx();
let h1 = hash(b"hello world!");

Expand Down Expand Up @@ -1424,7 +1424,7 @@ mod tests {

bank0.fill_bank_with_ticks_for_tests();
let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
poh_recorder.set_bank(&bank1, false);
poh_recorder.set_bank(bank1.clone(), false);

// Record up to exactly min tick height
let min_tick_height = poh_recorder.working_bank.as_ref().unwrap().min_tick_height;
Expand Down Expand Up @@ -1478,7 +1478,7 @@ mod tests {
Arc::new(AtomicBool::default()),
);

poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);
let num_ticks_to_max = bank.max_tick_height() - poh_recorder.tick_height;
for _ in 0..num_ticks_to_max {
poh_recorder.tick();
Expand Down Expand Up @@ -1518,7 +1518,7 @@ mod tests {
Arc::new(AtomicBool::default()),
);

poh_recorder.set_bank(&bank, true);
poh_recorder.set_bank(bank.clone(), true);
poh_recorder.tick();
assert_eq!(
poh_recorder
Expand Down Expand Up @@ -1592,7 +1592,7 @@ mod tests {

bank0.fill_bank_with_ticks_for_tests();
let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
poh_recorder.set_bank(&bank1, false);
poh_recorder.set_bank(bank1, false);

// Check we can make two ticks without hitting min_tick_height
let remaining_ticks_to_min =
Expand Down Expand Up @@ -1740,7 +1740,7 @@ mod tests {
Arc::new(AtomicBool::default()),
);

poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);
assert_eq!(bank.slot(), 0);
poh_recorder.reset(bank, Some((4, 4)));
assert!(poh_recorder.working_bank.is_none());
Expand Down Expand Up @@ -1772,7 +1772,7 @@ mod tests {
None,
Arc::new(AtomicBool::default()),
);
poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank, false);
poh_recorder.clear_bank();
assert!(receiver.try_recv().is_ok());
}
Expand Down Expand Up @@ -1807,7 +1807,7 @@ mod tests {
Arc::new(AtomicBool::default()),
);

poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);

// Simulate ticking much further than working_bank.max_tick_height
let max_tick_height = poh_recorder.working_bank.as_ref().unwrap().max_tick_height;
Expand Down Expand Up @@ -2102,7 +2102,7 @@ mod tests {
// Move the bank up a slot (so that max_tick_height > slot 0's tick_height)
let bank = Arc::new(Bank::new_from_parent(&bank, &Pubkey::default(), 1));
// If we set the working bank, the node should be leader within next 2 slots
poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);
assert!(poh_recorder.would_be_leader(2 * bank.ticks_per_slot()));
}
}
Expand Down Expand Up @@ -2136,7 +2136,7 @@ mod tests {
for _ in 0..(bank.ticks_per_slot() * 3) {
poh_recorder.tick();
}
poh_recorder.set_bank(&bank, false);
poh_recorder.set_bank(bank.clone(), false);
assert!(!bank.is_hash_valid_for_age(&genesis_hash, 0));
assert!(bank.is_hash_valid_for_age(&genesis_hash, 1));
}
Expand Down
2 changes: 1 addition & 1 deletion poh/src/poh_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ mod tests {
hashes_per_batch,
record_receiver,
);
poh_recorder.write().unwrap().set_bank(&bank, false);
poh_recorder.write().unwrap().set_bank(bank, false);

// get some events
let mut hashes = 0;
Expand Down