From ec1e5d1a459946789082abd337e7eeb2df309c26 Mon Sep 17 00:00:00 2001 From: arkpar Date: Fri, 3 Jan 2020 10:59:18 +0100 Subject: [PATCH 1/3] Account for references when pinning --- client/state-db/src/noncanonical.rs | 64 ++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/client/state-db/src/noncanonical.rs b/client/state-db/src/noncanonical.rs index 5db47cc21906d..94043faae6945 100644 --- a/client/state-db/src/noncanonical.rs +++ b/client/state-db/src/noncanonical.rs @@ -83,6 +83,8 @@ fn discard_values( if let Some(ref mut into) = into { into.insert(key, value); } + } else if let Some(ref mut into) = into { + into.insert(e.key().clone(), e.get().1.clone()); } }, Entry::Vacant(_) => { @@ -801,7 +803,7 @@ mod tests { fn keeps_pinned() { let mut db = make_db(&[]); - // - 1 - 1_1 + // - 0 - 1_1 // \ 1_2 let (h_1, c_1) = (H256::random(), make_changeset(&[1], &[])); @@ -821,4 +823,64 @@ mod tests { overlay.unpin(&h_1); assert!(!contains(&overlay, 1)); } + + #[test] + fn keeps_pinned_ref_count() { + let mut db = make_db(&[]); + + // - 0 - 1_1 + // \ 1_2 + // \ 1_3 + + // 1_1 and 1_2 both make the same change + let (h_1, c_1) = (H256::random(), make_changeset(&[1], &[])); + let (h_2, c_2) = (H256::random(), make_changeset(&[1], &[])); + let (h_3, c_3) = (H256::random(), make_changeset(&[], &[])); + + let mut overlay = NonCanonicalOverlay::::new(&db).unwrap(); + db.commit(&overlay.insert::(&h_1, 1, &H256::default(), c_1).unwrap()); + db.commit(&overlay.insert::(&h_2, 1, &H256::default(), c_2).unwrap()); + db.commit(&overlay.insert::(&h_3, 1, &H256::default(), c_3).unwrap()); + + overlay.pin(&h_1); + + let mut commit = CommitSet::default(); + overlay.canonicalize::(&h_3, &mut commit).unwrap(); + db.commit(&commit); + overlay.apply_pending(); // 1_2 should be discarded, 1_1 is pinned + + assert!(contains(&overlay, 1)); + overlay.unpin(&h_1); + assert!(!contains(&overlay, 1)); + } + + #[test] + fn pin_keeps_parent() { + let mut db = make_db(&[]); + + // - 0 - 1_1 - 2_1 + // \ 1_2 + + // 1_1 and 1_2 both make the same change + let (h_11, c_11) = (H256::random(), make_changeset(&[1], &[])); + let (h_12, c_12) = (H256::random(), make_changeset(&[], &[])); + let (h_21, c_21) = (H256::random(), make_changeset(&[], &[])); + + let mut overlay = NonCanonicalOverlay::::new(&db).unwrap(); + db.commit(&overlay.insert::(&h_11, 1, &H256::default(), c_11).unwrap()); + db.commit(&overlay.insert::(&h_12, 1, &H256::default(), c_12).unwrap()); + db.commit(&overlay.insert::(&h_21, 2, &h_11, c_21).unwrap()); + + overlay.pin(&h_21); + + let mut commit = CommitSet::default(); + overlay.canonicalize::(&h_12, &mut commit).unwrap(); + db.commit(&commit); + overlay.apply_pending(); // 1_1 and 2_1 should be both pinned + + assert!(contains(&overlay, 1)); + overlay.unpin(&h_21); + assert!(!contains(&overlay, 1)); + assert!(overlay.pinned.is_empty()); + } } From 6b63d43e76f782be63a139e9fef6658bb96f1ad1 Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 7 Jan 2020 14:59:21 +0100 Subject: [PATCH 2/3] Fixed pinned state issues --- client/state-db/src/noncanonical.rs | 86 +++++++++++++++++++---------- 1 file changed, 57 insertions(+), 29 deletions(-) diff --git a/client/state-db/src/noncanonical.rs b/client/state-db/src/noncanonical.rs index 94043faae6945..b1fde4e461268 100644 --- a/client/state-db/src/noncanonical.rs +++ b/client/state-db/src/noncanonical.rs @@ -37,7 +37,9 @@ pub struct NonCanonicalOverlay { pending_canonicalizations: Vec, pending_insertions: Vec, values: HashMap, //ref counted - pinned: HashMap>, //would be deleted but kept around because block is pinned + //would be deleted but kept around because block is pinned, ref counted. + pinned: HashMap, + pinned_insertions: HashMap>, } #[derive(Encode, Decode)] @@ -68,23 +70,14 @@ fn insert_values(values: &mut HashMap, inserted: } } -fn discard_values( - values: &mut HashMap, - inserted: Vec, - mut into: Option<&mut HashMap>, -) { +fn discard_values(values: &mut HashMap, inserted: Vec) { for k in inserted { match values.entry(k) { Entry::Occupied(mut e) => { let (ref mut counter, _) = e.get_mut(); *counter -= 1; if *counter == 0 { - let (key, (_, value)) = e.remove_entry(); - if let Some(ref mut into) = into { - into.insert(key, value); - } - } else if let Some(ref mut into) = into { - into.insert(e.key().clone(), e.get().1.clone()); + e.remove_entry(); } }, Entry::Vacant(_) => { @@ -99,7 +92,8 @@ fn discard_descendants( mut values: &mut HashMap, index: usize, parents: &mut HashMap, - pinned: &mut HashMap>, + pinned: &HashMap, + pinned_insertions: &mut HashMap>, hash: &BlockHash, ) { let mut discarded = Vec::new(); @@ -107,9 +101,15 @@ fn discard_descendants( *level = level.drain(..).filter_map(|overlay| { let parent = parents.get(&overlay.hash).expect("there is a parent entry for each entry in levels; qed").clone(); if parent == *hash { - parents.remove(&overlay.hash); - discarded.push(overlay.hash); - discard_values(&mut values, overlay.inserted, pinned.get_mut(hash)); + discarded.push(overlay.hash.clone()); + if pinned.contains_key(hash) { + // save to be discarded later. + pinned_insertions.insert(hash.clone(), overlay.inserted); + } else { + // discard immediatelly. + parents.remove(&overlay.hash); + discard_values(&mut values, overlay.inserted); + } None } else { Some(overlay) @@ -117,7 +117,7 @@ fn discard_descendants( }).collect(); } for hash in discarded { - discard_descendants(levels, values, index + 1, parents, pinned, &hash); + discard_descendants(levels, values, index + 1, parents, pinned, pinned_insertions, &hash); } } @@ -178,6 +178,7 @@ impl NonCanonicalOverlay { pending_canonicalizations: Default::default(), pending_insertions: Default::default(), pinned: Default::default(), + pinned_insertions: Default::default(), values: values, }) } @@ -341,18 +342,23 @@ impl NonCanonicalOverlay { // discard unfinalized overlays and values for (i, overlay) in level.into_iter().enumerate() { - self.parents.remove(&overlay.hash); if i != index { discard_descendants( &mut self.levels, &mut self.values, 0, &mut self.parents, - &mut self.pinned, + &self.pinned, + &mut self.pinned_insertions, &overlay.hash, ); } - discard_values(&mut self.values, overlay.inserted, self.pinned.get_mut(&overlay.hash)); + if self.pinned.contains_key(&overlay.hash) { + self.pinned_insertions.insert(overlay.hash.clone(), overlay.inserted); + } else { + self.parents.remove(&overlay.hash); + discard_values(&mut self.values, overlay.inserted); + } } } if let Some(hash) = last { @@ -366,11 +372,6 @@ impl NonCanonicalOverlay { if let Some((_, value)) = self.values.get(&key) { return Some(value.clone()); } - for pinned in self.pinned.values() { - if let Some(value) = pinned.get(&key) { - return Some(value.clone()); - } - } None } @@ -387,7 +388,7 @@ impl NonCanonicalOverlay { for overlay in level.into_iter() { commit.meta.deleted.push(overlay.journal_key); self.parents.remove(&overlay.hash); - discard_values(&mut self.values, overlay.inserted, None); + discard_values(&mut self.values, overlay.inserted); } commit }) @@ -404,7 +405,7 @@ impl NonCanonicalOverlay { .expect("Hash is added in insert"); let overlay = self.levels[level_index].pop().expect("Empty levels are not allowed in self.levels"); - discard_values(&mut self.values, overlay.inserted, None); + discard_values(&mut self.values, overlay.inserted); if self.levels[level_index].is_empty() { debug_assert_eq!(level_index, self.levels.len() - 1); self.levels.pop_back(); @@ -426,12 +427,39 @@ impl NonCanonicalOverlay { /// Pin state values in memory pub fn pin(&mut self, hash: &BlockHash) { - self.pinned.insert(hash.clone(), HashMap::default()); + // Also pin all parents + let mut parent = Some(hash); + while let Some(hash) = parent { + let refs = self.pinned.entry(hash.clone()).or_default(); + if *refs == 0 { + trace!(target: "state-db", "Pinned non-canon block: {:?}", hash); + } + *refs += 1; + parent = self.parents.get(hash); + } } /// Discard pinned state pub fn unpin(&mut self, hash: &BlockHash) { - self.pinned.remove(hash); + // Also unpin all parents + let mut parent = Some(hash.clone()); + while let Some(hash) = parent { + parent = self.parents.get(&hash).cloned(); + match self.pinned.entry(hash.clone()) { + Entry::Occupied(mut entry) => { + *entry.get_mut() -= 1; + if *entry.get() == 0 { + entry.remove(); + if let Some(inserted) = self.pinned_insertions.remove(&hash) { + trace!(target: "state-db", "Discarding unpinned non-canon block: {:?}", hash); + discard_values(&mut self.values, inserted); + self.parents.remove(&hash); + } + } + }, + Entry::Vacant(_) => {}, + } + } } } From d35dc7045ef2779994d938d1625fbcea110b9749 Mon Sep 17 00:00:00 2001 From: arkpar Date: Sat, 11 Jan 2020 13:50:00 +0100 Subject: [PATCH 3/3] Fixes --- client/state-db/src/noncanonical.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/client/state-db/src/noncanonical.rs b/client/state-db/src/noncanonical.rs index b1fde4e461268..373c1aa0da076 100644 --- a/client/state-db/src/noncanonical.rs +++ b/client/state-db/src/noncanonical.rs @@ -102,9 +102,9 @@ fn discard_descendants( let parent = parents.get(&overlay.hash).expect("there is a parent entry for each entry in levels; qed").clone(); if parent == *hash { discarded.push(overlay.hash.clone()); - if pinned.contains_key(hash) { + if pinned.contains_key(&overlay.hash) { // save to be discarded later. - pinned_insertions.insert(hash.clone(), overlay.inserted); + pinned_insertions.insert(overlay.hash.clone(), overlay.inserted); } else { // discard immediatelly. parents.remove(&overlay.hash); @@ -427,6 +427,10 @@ impl NonCanonicalOverlay { /// Pin state values in memory pub fn pin(&mut self, hash: &BlockHash) { + if self.pending_insertions.contains(hash) { + debug_assert!(false, "Trying to pin pending state"); + return; + } // Also pin all parents let mut parent = Some(hash); while let Some(hash) = parent { @@ -840,6 +844,7 @@ mod tests { let mut overlay = NonCanonicalOverlay::::new(&db).unwrap(); db.commit(&overlay.insert::(&h_1, 1, &H256::default(), c_1).unwrap()); db.commit(&overlay.insert::(&h_2, 1, &H256::default(), c_2).unwrap()); + overlay.apply_pending(); overlay.pin(&h_1); @@ -869,6 +874,7 @@ mod tests { db.commit(&overlay.insert::(&h_1, 1, &H256::default(), c_1).unwrap()); db.commit(&overlay.insert::(&h_2, 1, &H256::default(), c_2).unwrap()); db.commit(&overlay.insert::(&h_3, 1, &H256::default(), c_3).unwrap()); + overlay.apply_pending(); overlay.pin(&h_1); @@ -889,7 +895,6 @@ mod tests { // - 0 - 1_1 - 2_1 // \ 1_2 - // 1_1 and 1_2 both make the same change let (h_11, c_11) = (H256::random(), make_changeset(&[1], &[])); let (h_12, c_12) = (H256::random(), make_changeset(&[], &[])); let (h_21, c_21) = (H256::random(), make_changeset(&[], &[])); @@ -898,6 +903,7 @@ mod tests { db.commit(&overlay.insert::(&h_11, 1, &H256::default(), c_11).unwrap()); db.commit(&overlay.insert::(&h_12, 1, &H256::default(), c_12).unwrap()); db.commit(&overlay.insert::(&h_21, 2, &h_11, c_21).unwrap()); + overlay.apply_pending(); overlay.pin(&h_21);