Unverified Commit cf6e7106 authored by Michael Müller's avatar Michael Müller Committed by GitHub

Implement storage2::Stash::remove_occupied unsafe function (#440)

* [core] Add stash::remove()

* [core] Add benchmarks: take() vs remove()

* [core] Add benchmark for worst case of take()

* [core] Make worst case test even worse

* [core] Add assertions

* [core] Rename stash.remove() to stash.remove_occupied() and make it unsafe

* [core] Add code for linking list entries when removing one

* [core] Add test for remove with multiple push/pulls

* [core] Improve code

* [core] Do not actually read value in remove_occupied()

* [core] Make clippy happy

* [core] Upgrade criterion

* [core] Remove unnecessary code branches

* [core] Refactor logic

* [core] Rename file

* [core] Remove criterion temporarily

* Add new ops for storage2/vec (#453)

* Fix typo

* [core] Add vec.set()

* [core] Add vec.clear()

* [core] Add benchmarks for vec.put() and vec.clear()

* [core] Minor improvements

* [core] Add user comment

* [core] Improve style

* [core] Add worst case benches

* [core] Include comments from review

* [core] Apply cargo fmt

* [core] Remove unnecessary variable

* [core] Include put worst case bench

* [core] Rename benches to lazy vs. cached

* [core] Reduce benchmark overhead by preventing cache writes

* [core] Add black boxes to prevent over optimization

* [core] improve benchmarks for new storage vector operations (#457)

* [core] optimize LazyIndexMap::put

* [core] put some forgotten black_box instances

* [core] further minor improvements to the benchmarks

* [core] Add criterion dependency workaround

* [core] Change vec.set() to return Result<(), IndexOutOfBounds>

* [core] Remove outdated comment

* [core] Minor fixes

* [core] Minor test fixes

* Apply suggestions from code review
Co-authored-by: default avatarHero Bird <robin.freyler@gmail.com>
Co-authored-by: default avatarHero Bird <robin.freyler@gmail.com>

* [core] Structure and update benches

* [core] Address 'unused Result that must be used'

* [core] Clean up code

* [core] Remove outdated files

* [core] Apply cargo fmt

* Apply suggestions from code review
Co-authored-by: default avatarHero Bird <robin.freyler@gmail.com>

* [core] Wrap comment

* [core] Constrain mutability
Co-authored-by: default avatarHero Bird <robin.freyler@gmail.com>
parent 9eb92645
Pipeline #97941 failed with stages
in 8 minutes and 10 seconds
......@@ -72,5 +72,9 @@ std = [
]
[[bench]]
name = "bench"
name = "bench_vec"
harness = false
[[bench]]
name = "bench_stash"
harness = false
// Copyright 2019-2020 Parity Technologies (UK) Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
use criterion::{
black_box,
criterion_group,
criterion_main,
Criterion,
};
use ink_core::{
env,
storage2::{
collections::Stash as StorageStash,
traits::{
KeyPtr,
SpreadLayout,
},
},
};
use ink_primitives::Key;
criterion_group!(populated_cache, bench_remove_occupied_populated_cache,);
criterion_group!(empty_cache, bench_remove_occupied_empty_cache,);
criterion_main!(populated_cache, empty_cache,);
/// Returns some test values for use in benchmarks.
#[rustfmt::skip]
fn test_values() -> &'static [u8] {
&[
b'0', b'1', b'2', b'3', b'4', b'5', b'6', b'7', b'8', b'9',
b'A', b'B', b'C', b'D', b'E', b'F'
]
}
/// Creates a storage stash from the given slice.
fn storage_stash_from_slice(slice: &[u8]) -> StorageStash<u8> {
slice.iter().copied().collect::<StorageStash<u8>>()
}
/// Creates a storage stash and pushes it to the contract storage.
fn push_storage_stash() {
let stash = storage_stash_from_slice(test_values());
let root_key = Key::from([0x00; 32]);
SpreadLayout::push_spread(&stash, &mut KeyPtr::from(root_key));
}
/// Pulls a lazily loading storage stash instance from the contract storage.
fn pull_storage_stash() -> StorageStash<u8> {
let root_key = Key::from([0x00; 32]);
<StorageStash<u8> as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key))
}
mod populated_cache {
use super::*;
pub fn remove_occupied_all(test_values: &[u8]) {
let mut stash = storage_stash_from_slice(test_values);
for (index, _value) in test_values.iter().enumerate() {
black_box(unsafe { stash.remove_occupied(index as u32) });
}
}
pub fn take_all(test_values: &[u8]) {
let mut stash = storage_stash_from_slice(test_values);
for (index, _value) in test_values.iter().enumerate() {
black_box(stash.take(index as u32));
}
}
}
fn bench_remove_occupied_populated_cache(c: &mut Criterion) {
let mut group = c.benchmark_group(
"Compare: `remove_occupied_all` and `take_all` (populated cache)",
);
let test_values = [b'A', b'B', b'C', b'D', b'E', b'F'];
group.bench_with_input("remove_occupied_all", &test_values, |b, i| {
b.iter(|| populated_cache::remove_occupied_all(i))
});
group.bench_with_input("take_all", &test_values, |b, i| {
b.iter(|| populated_cache::take_all(i))
});
group.finish();
}
mod empty_cache {
use super::*;
/// In this case we lazily load the stash from storage using `pull_spread`.
/// This will just load lazily and won't pull anything from the storage.
pub fn remove_occupied_all() {
push_storage_stash();
let mut stash = pull_storage_stash();
for index in 0..stash.len() {
black_box(unsafe { stash.remove_occupied(index) });
}
}
/// In this case we lazily load the stash from storage using `pull_spread`.
/// This will just load lazily and won't pull anything from the storage.
/// `take` will then result in loading from storage.
pub fn take_all() {
push_storage_stash();
let mut stash = pull_storage_stash();
for index in 0..stash.len() {
black_box(stash.take(index));
}
}
}
/// In this case we lazily instantiate a `StorageStash` by first `create_and_store`-ing
/// into the contract storage. We then load the stash from storage lazily in each
/// benchmark iteration.
fn bench_remove_occupied_empty_cache(c: &mut Criterion) {
let _ = env::test::run_test::<env::DefaultEnvTypes, _>(|_| {
let mut group = c.benchmark_group(
"Compare: `remove_occupied_all` and `take_all` (empty cache)",
);
group.bench_function("remove_occupied_all", |b| {
b.iter(|| empty_cache::remove_occupied_all())
});
group.bench_function("take_all", |b| b.iter(|| empty_cache::take_all()));
group.finish();
Ok(())
})
.unwrap();
}
......@@ -84,7 +84,7 @@ mod populated_cache {
pub fn set(test_values: &[u8]) {
let mut vec = storage_vec_from_slice(&test_values);
for (index, _value) in test_values.iter().enumerate() {
black_box(vec.set(index as u32, b'X'));
let _ = black_box(vec.set(index as u32, b'X'));
}
}
......@@ -159,7 +159,7 @@ mod empty_cache {
push_storage_vec();
let mut vec = pull_storage_vec();
for index in 0..vec.len() {
black_box(vec.set(index, b'X'));
let _ = black_box(vec.set(index, b'X'));
}
}
}
......
......@@ -344,49 +344,17 @@ where
}
}
/// Put the element into the stash at the next vacant position.
/// Returns the previous and next vacant entry for the entry at index `at`.
///
/// Returns the stash index that the element was put into.
pub fn put(&mut self, new_value: T) -> Index {
let new_entry = Some(Entry::Occupied(new_value));
let new_index = if let Some(index) = self.last_vacant_index() {
// Put the new element to the most recent vacant index if not all entries are occupied.
let old_entry = self
.entries
.put_get(index, new_entry)
.expect("a `last_vacant_index()` must point to an occupied cell");
let vacant_entry = match old_entry {
Entry::Vacant(vacant_entry) => vacant_entry,
Entry::Occupied(_) => {
unreachable!("`last_vacant_index()` must point to a vacant entry")
}
};
self.remove_vacant_entry(index, vacant_entry);
index
} else {
// Push the new element to the end if all entries are occupied.
let new_index = self.header.len_entries;
self.entries.put(new_index, new_entry);
self.header.last_vacant += 1;
self.header.len_entries += 1;
new_index
};
self.header.len += 1;
new_index
}
/// Takes the element stored at the given index if any.
pub fn take(&mut self, at: Index) -> Option<T> {
// Cases:
// - There are vacant entries already.
// - There are no vacant entries before.
if at >= self.len_entries() {
// Early return since `at` index is out of bounds.
return None
}
// Precompute prev and next vacant entries as we might need them later.
// Due to borrow checker constraints we cannot have this at a later stage.
let (prev, next) = if let Some(index) = self.last_vacant_index() {
/// If there exists a last vacant entry, the return value is a tuple
/// `(index_of_previous_vacant, index_of_next_vacant)`.
/// The two `index_` values hereby are selected in a way that makes it
/// more likely that the stash is refilled from low indices.
///
/// If no vacant entry exists a self-referential tuple of `(at, at)`
/// is returned.
fn fetch_prev_and_next_vacant_entry(&self, at: Index) -> (Index, Index) {
if let Some(index) = self.last_vacant_index() {
let root_vacant = self
.entries
.get(index)
......@@ -411,16 +379,16 @@ where
// Default prev and next to the given at index.
// So the resulting vacant index is pointing to itself.
(at, at)
};
let entry_mut = self.entries.get_mut(at).expect("index is out of bounds");
if entry_mut.is_vacant() {
// Early return if the taken entry is already vacant.
return None
}
// At this point we know that the entry is occupied with a value.
let new_vacant_entry = Entry::Vacant(VacantEntry { prev, next });
let taken_entry = core::mem::replace(entry_mut, new_vacant_entry);
// Update links from and to neighbouring vacant entries.
}
/// Updates links from and to neighbouring vacant entries.
fn update_neighboring_vacant_entry_links(
&mut self,
prev: Index,
next: Index,
at: Index,
) {
if prev == next {
// Previous and next are the same so we can update the vacant
// neighbour with a single look-up.
......@@ -448,6 +416,60 @@ where
.expect("`next` must point to an existing vacant entry at this point")
.prev = at;
}
}
/// Put the element into the stash at the next vacant position.
///
/// Returns the stash index that the element was put into.
pub fn put(&mut self, new_value: T) -> Index {
let new_entry = Some(Entry::Occupied(new_value));
let new_index = if let Some(index) = self.last_vacant_index() {
// Put the new element to the most recent vacant index if not all entries are occupied.
let old_entry = self
.entries
.put_get(index, new_entry)
.expect("a `last_vacant_index()` must point to an occupied cell");
let vacant_entry = match old_entry {
Entry::Vacant(vacant_entry) => vacant_entry,
Entry::Occupied(_) => {
unreachable!("`last_vacant_index()` must point to a vacant entry")
}
};
self.remove_vacant_entry(index, vacant_entry);
index
} else {
// Push the new element to the end if all entries are occupied.
let new_index = self.header.len_entries;
self.entries.put(new_index, new_entry);
self.header.last_vacant += 1;
self.header.len_entries += 1;
new_index
};
self.header.len += 1;
new_index
}
/// Takes the element stored at the given index if any.
pub fn take(&mut self, at: Index) -> Option<T> {
// Cases:
// - There are vacant entries already.
// - There are no vacant entries before.
if at >= self.len_entries() {
// Early return since `at` index is out of bounds.
return None
}
// Precompute prev and next vacant entries as we might need them later.
// Due to borrow checker constraints we cannot have this at a later stage.
let (prev, next) = self.fetch_prev_and_next_vacant_entry(at);
let entry_mut = self.entries.get_mut(at).expect("index is out of bounds");
if entry_mut.is_vacant() {
// Early return if the taken entry is already vacant.
return None
}
// At this point we know that the entry is occupied with a value.
let new_vacant_entry = Entry::Vacant(VacantEntry { prev, next });
let taken_entry = core::mem::replace(entry_mut, new_vacant_entry);
self.update_neighboring_vacant_entry_links(prev, next, at);
// Take the value out of the taken occupied entry and return it.
match taken_entry {
Entry::Occupied(value) => {
......@@ -463,6 +485,43 @@ where
}
}
/// Removes the element stored at the given index if any.
///
/// This method acts similar to the take API and even still returns an Option.
/// However, it guarantees to make no contract storage reads to the indexed
/// element and will only write to its internal low-level lazy cache that the
/// element at the given index is going to be removed at the end of the contract
/// execution.
///
/// Calling this method with an index out of bounds for the returns `None` and
/// does not `remove` the element, otherwise it returns `Some(())`.
///
/// # Safety
///
/// The caller must ensure that `at` refers to an occupied index. Behavior is
/// unspecified if `at` refers to a vacant index and could seriously damage the
/// contract storage integrity.
pub unsafe fn remove_occupied(&mut self, at: Index) -> Option<()> {
// This function is written similar to [`Stash::take`], with the exception
// that the caller has to ensure that `at` refers to an occupied entry whereby
// the procedure can avoid loading the occupied entry which might be handy if
// the stored `T` is especially costly to load from contract storage.
if at >= self.len_entries() {
// Early return since `at` index is out of bounds.
return None
}
// Precompute prev and next vacant entries as we might need them later.
// Due to borrow checker constraints we cannot have this at a later stage.
let (prev, next) = self.fetch_prev_and_next_vacant_entry(at);
let new_vacant_entry = Entry::Vacant(VacantEntry { prev, next });
self.entries.put(at, Some(new_vacant_entry));
self.update_neighboring_vacant_entry_links(prev, next, at);
use core::cmp::min;
self.header.last_vacant = min(self.header.last_vacant, min(at, min(prev, next)));
self.header.len -= 1;
Some(())
}
/// Defragments the underlying storage to minimize footprint.
///
/// Returns the number of storage cells freed this way.
......
......@@ -106,6 +106,71 @@ fn take_out_of_bounds_works() {
assert_eq!(stash.take(3), None);
}
#[test]
fn remove_from_filled_works() {
let test_values = [b'A', b'B', b'C', b'D', b'E', b'F'];
let mut stash = test_values.iter().copied().collect::<StorageStash<_>>();
let mut count = stash.len();
for (index, val) in test_values.iter().enumerate() {
let index = index as u32;
assert_eq!(stash.get(index), Some(val));
assert_eq!(unsafe { stash.remove_occupied(index) }, Some(()));
assert_eq!(stash.get(index), None);
count -= 1;
assert_eq!(stash.len(), count);
}
assert_eq!(stash.len(), 0);
}
#[test]
fn remove_from_empty_works() {
let mut stash = <StorageStash<u8>>::new();
assert_eq!(unsafe { stash.remove_occupied(0) }, None);
}
#[test]
fn remove_out_of_bounds_works() {
let mut stash = [b'A', b'B', b'C']
.iter()
.copied()
.collect::<StorageStash<_>>();
assert_eq!(unsafe { stash.remove_occupied(3) }, None);
}
#[test]
fn remove_works_with_spread_layout_push_pull() -> env::Result<()> {
env::test::run_test::<env::DefaultEnvTypes, _>(|_| {
// First populate some storage Stash and writes that to the contract storage using pull_spread
// and some known Key.
let stash = [b'A', b'B', b'C']
.iter()
.copied()
.collect::<StorageStash<_>>();
let root_key = Key::from([0x00; 32]);
SpreadLayout::push_spread(&stash, &mut KeyPtr::from(root_key));
// Then load another instance from the same key lazily and remove some of
// the known-to-be-populated entries from it. Afterwards push_spread this second instance and
// load yet another using pull_spread again.
let mut stash2 =
<StorageStash<u8> as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key));
assert_eq!(unsafe { stash2.remove_occupied(0) }, Some(()));
SpreadLayout::push_spread(&stash2, &mut KeyPtr::from(root_key));
// This time we check from the third instance using
// get if the expected cells are still there or have been successfully removed.
let stash3 =
<StorageStash<u8> as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key));
assert_eq!(stash3.get(0), None);
assert_eq!(stash3.get(1), Some(&b'B'));
assert_eq!(stash3.get(2), Some(&b'C'));
assert_eq!(stash3.len(), 2);
Ok(())
})
}
#[test]
fn get_works() {
let test_values = [b'A', b'B', b'C', b'D', b'E', b'F'];
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment