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

Remove needless last_root for better reclaims #8148

Merged
merged 3 commits into from
Feb 12, 2020
Merged

Remove needless last_root for better reclaims #8148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9e10ef
Restore last_root to fix unintended storage delete
ryoqun Feb 6, 2020
3120504
Remove last_root thing altogether
ryoqun Feb 8, 2020
da1af6c
Remove unneeded test...
ryoqun Feb 8, 2020
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
23 changes: 8 additions & 15 deletions runtime/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,8 +671,6 @@ impl AccountsDB {
reclaims.extend(new_reclaims);
}

let last_root = accounts_index.last_root;

drop(accounts_index);
drop(storage);

Expand All @@ -683,16 +681,16 @@ impl AccountsDB {
}
}

self.handle_reclaims(&reclaims, last_root);
self.handle_reclaims(&reclaims);
}

fn handle_reclaims(&self, reclaims: &[(Slot, AccountInfo)], last_root: Slot) {
fn handle_reclaims(&self, reclaims: &[(Slot, AccountInfo)]) {
let mut dead_accounts = Measure::start("reclaims::remove_dead_accounts");
let mut dead_slots = self.remove_dead_accounts(reclaims);
dead_accounts.stop();

let mut cleanup_dead_slots = Measure::start("reclaims::purge_slots");
self.cleanup_dead_slots(&mut dead_slots, last_root);
self.cleanup_dead_slots(&mut dead_slots);
cleanup_dead_slots.stop();

let mut purge_slots = Measure::start("reclaims::purge_slots");
Expand Down Expand Up @@ -1097,7 +1095,7 @@ impl AccountsDB {
slot_id: Slot,
infos: Vec<AccountInfo>,
accounts: &[(&Pubkey, &Account)],
) -> (Vec<(Slot, AccountInfo)>, u64) {
) -> Vec<(Slot, AccountInfo)> {
let mut reclaims: Vec<(Slot, AccountInfo)> = Vec::with_capacity(infos.len() * 2);
let index = self.accounts_index.read().unwrap();
let mut update_index_work = Measure::start("update_index_work");
Expand All @@ -1112,7 +1110,6 @@ impl AccountsDB {
})
.collect();

let last_root = index.last_root;
drop(index);
if !inserts.is_empty() {
let mut index = self.accounts_index.write().unwrap();
Expand All @@ -1121,7 +1118,7 @@ impl AccountsDB {
}
}
update_index_work.stop();
(reclaims, last_root)
reclaims
}

fn remove_dead_accounts(&self, reclaims: &[(Slot, AccountInfo)]) -> HashSet<Slot> {
Expand Down Expand Up @@ -1156,9 +1153,7 @@ impl AccountsDB {
dead_slots
}

fn cleanup_dead_slots(&self, dead_slots: &mut HashSet<Slot>, last_root: u64) {
// a slot is not totally dead until it is older than the root
dead_slots.retain(|slot| *slot < last_root);
Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized all inputs of this function is ensured to be older or equal to last_root.

Notice the or equal to part, this assertion must be *slot <= last_root for purge_zero_lamport_accounts. But, we don't need this to begin with, as said above.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, why are they guaranteed to be older than last_root?

Copy link
Member Author

@ryoqun ryoqun Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I can explain this by reading the code from here upwards the call graph.

Firstly, this cleanup_dead_slots(dead_slots, last_root) is only called from handle_reclaims(reclaims).
dead_slots are derived from reclaims by calling remove_dead_accounts() inside handle_reclaims(), which does just simple 1-to-1 storage -> slot mapping. Thus, we just need to explain in terms of reclaims further on.

handle_reclaims() are called only from purge_zero_lamport_accounts()(1) and store_with_hashes() (2). These are the only 2 code paths reaching to this function.

(1) For the purge_zero_lamport_accounts() cod epath, reasoning is simple:

The reclaims is a collection of results of accounts_index.purge(&pubkey), which in turn the sum of get_rooted_entries() for all such zero lamport pubkeys. So, reclaims (thus, dead_slots) are always rooted slots only. This is true for normal operation because the validator calls add_root() as it roots slots.

Also this is also true for snapshot restoration because generate_index() is called before purge_zero_lamport_accounts() and generate_index() does accounts_index.roots.insert(*slot_id).

(2) For the store_with_hashes(), reasoning is a bit longer:

The reclaims is ultimately only generated at accounts_index.update(). There is intermediately fns like accounts_db.update_index() and accounts_index.insert(), but the destination of this call graph is same.

Next, accounts_index.update(slot, ...) creates reclaims by concatenating two sets:
A: can_purge-able (= slot < max_root) entries
B: Old entries in slot by replacing with updated one

A is obviously fine because max_root is always member of accounts_index.roots. B is ensured not to be dead at remove_dead_accounts() because the case B is always inserting an updated alive entry into the slot.

So, as a normal operation, cleanup_dead_slots() never removes anything such that slot > last_root.

And leaving this line's retain causes halfway-deallocated state because cleanup_dead_slots() is only called once ever for each given slot. That's because later invocations of remove_dead_accounts() would remove any slots whose storage are already removed.

Thanks for reading a rather long comment. :)

So I think this retain does add little value and complicates code reading with seemingly overlapping and spread responsibility for ensuring purge-able slots at caller-site or at callee-site.

As a second thought, if you're a bit concerned for future changes that might break these assumptions, this PR can be changed to assert! that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Thanks for the description. I think it can be removed.

fn cleanup_dead_slots(&self, dead_slots: &mut HashSet<Slot>) {
if !dead_slots.is_empty() {
{
let mut index = self.accounts_index.write().unwrap();
Expand Down Expand Up @@ -1219,11 +1214,11 @@ impl AccountsDB {
store_accounts.stop();

let mut update_index = Measure::start("store::update_index");
let (reclaims, last_root) = self.update_index(slot_id, infos, accounts);
let reclaims = self.update_index(slot_id, infos, accounts);
update_index.stop();
trace!("reclaim: {}", reclaims.len());

self.handle_reclaims(&reclaims, last_root);
self.handle_reclaims(&reclaims);
}

pub fn add_root(&self, slot: Slot) {
Expand Down Expand Up @@ -1782,9 +1777,7 @@ pub mod tests {
let (list, idx) = index.get(&pubkey, &ancestors).unwrap();
list[idx].1.store_id
};
//slot 0 is behind root, but it is not root, therefore it is purged
accounts.add_root(1);
assert!(accounts.accounts_index.read().unwrap().is_purged(0));

//slot is still there, since gc is lazy
assert!(accounts.storage.read().unwrap().0[&0].get(&id).is_some());
Expand Down
35 changes: 0 additions & 35 deletions runtime/src/accounts_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ pub struct AccountsIndex<T> {
pub account_maps: HashMap<Pubkey, RwLock<SlotList<T>>>,

pub roots: HashSet<Slot>,

// This value that needs to be stored to recover the index from AppendVec
pub last_root: Slot,
}

impl<T: Clone> AccountsIndex<T> {
Expand Down Expand Up @@ -148,10 +145,6 @@ impl<T: Clone> AccountsIndex<T> {
entry.write().unwrap().push((slot, account_info));
}

pub fn is_purged(&self, slot: Slot) -> bool {
slot < self.last_root
}

pub fn can_purge(max_root: Slot, slot: Slot) -> bool {
slot < max_root
}
Expand All @@ -161,11 +154,6 @@ impl<T: Clone> AccountsIndex<T> {
}

pub fn add_root(&mut self, slot: Slot) {
assert!(
(self.last_root == 0 && slot == 0) || (slot >= self.last_root),
Copy link
Member Author

Choose a reason for hiding this comment

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

I think removing this assertion is ok to remove last_root, first of all.

"new roots must be increasing"
);
self.last_root = slot;
self.roots.insert(slot);
}
/// Remove the slot when the storage for the slot is freed
Expand Down Expand Up @@ -270,29 +258,6 @@ mod tests {
assert_eq!(list[idx], (0, true));
}

#[test]
fn test_is_purged() {
let mut index = AccountsIndex::<bool>::default();
assert!(!index.is_purged(0));
index.add_root(1);
assert!(index.is_purged(0));
}

#[test]
fn test_max_last_root() {
let mut index = AccountsIndex::<bool>::default();
index.add_root(1);
assert_eq!(index.last_root, 1);
}

#[test]
#[should_panic]
fn test_max_last_root_old() {
let mut index = AccountsIndex::<bool>::default();
index.add_root(1);
index.add_root(0);
}

#[test]
fn test_cleanup_first() {
let mut index = AccountsIndex::<bool>::default();
Expand Down