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

Reduce alloc in copy_to_overlay #178

Merged
merged 7 commits into from
Feb 6, 2023
Merged

Reduce alloc in copy_to_overlay #178

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
43331db
fix: reduce allocations in copy_to_overlay
hanabi1224 Feb 3, 2023
86ffde6
btree copy_to_overlay
hanabi1224 Feb 3, 2023
6e7ffb6
fix fmt and clippy
hanabi1224 Feb 3, 2023
ca9f8d7
rename
hanabi1224 Feb 4, 2023
59bcdde
mark TryFrom as #[cfg(test)]
hanabi1224 Feb 4, 2023
1af4855
revert LastKey
hanabi1224 Feb 4, 2023
3d40344
revert iter API change
hanabi1224 Feb 4, 2023
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 Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ snap = "1"
loom = { version = "0.5.1", optional = true }

[dev-dependencies]
env_logger = "0.9.0"
env_logger = "0.10.0"
fdlimit = "0.2.1"
rand = { version = "0.8.2", features = ["small_rng"] }
tempfile = "3.2"
Expand Down
32 changes: 18 additions & 14 deletions fuzz/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,68 +226,72 @@ pub trait DbSimulator {
};
Self::reset_model_from_database(&db.db, &mut layers, &old_layers);
},
Action::IterPrev =>
Action::IterPrev => {
if let Some(iter) = &mut db.iter {
let mut old_key = if let Some(old_key) = db.iter_current_key.take() {
old_key
} else {
retry_operation(|| iter.seek_to_last());
IterPosition::End
};
let new_key_value =
iter.prev().unwrap_or_else(|e| {
let new_key_value = iter
.prev()
.unwrap_or_else(|e| {
log::debug!("Database error: {}, restarting iter.prev without I/O limitations.", e);

// We ignore the error and reset the iterator
parity_db::set_number_of_allowed_io_operations(usize::MAX);
iter.seek_to_last().unwrap();
old_key = IterPosition::End;
iter.prev().unwrap()
});
})
.map(|(k, v)| (k.value().clone(), v.value().clone()));
let expected = Self::valid_iter_value(old_key, &layers, Ordering::Greater);
log::info!(
"Prev lookup on iterator with old position {:?}, expecting one of {:?}",
old_key,
expected
);
assert!(expected.contains(&new_key_value), "Prev lookup on iterator with old position {:?}, expecting one of {:?}, found {:?}",
old_key,
expected, new_key_value);
assert!(expected.contains(&new_key_value), "{}", "Prev lookup on iterator with old position {old_key:?}, expecting one of {expected:?}, found {new_key_value:?}");
db.iter_current_key = Some(
new_key_value
.map_or(IterPosition::Start, |(k, _)| IterPosition::Value(k[0])),
);
},
Action::IterNext =>
}
},
Action::IterNext => {
if let Some(iter) = &mut db.iter {
let mut old_key = if let Some(old_key) = db.iter_current_key.take() {
old_key
} else {
retry_operation(|| iter.seek_to_first());
IterPosition::Start
};
let new_key_value =
iter.next().unwrap_or_else(|e| {
let new_key_value = iter
.next()
.unwrap_or_else(|e| {
log::debug!("Database error: {}, restarting iter.next without I/O limitations.", e);

// We ignore the error and reset the iterator
parity_db::set_number_of_allowed_io_operations(usize::MAX);
iter.seek_to_first().unwrap();
old_key = IterPosition::Start;
iter.next().unwrap()
});
})
.map(|(k, v)| (k.value().clone(), v.value().clone()));
let expected = Self::valid_iter_value(old_key, &layers, Ordering::Less);
log::info!(
"Next lookup on iterator with old position {:?}, expecting one of {:?}",
old_key,
expected
);
assert!(expected.contains(&new_key_value), "Next lookup on iterator with old position {:?}, expecting one of {:?}, found {:?}", old_key, expected, new_key_value);
assert!(expected.contains(&new_key_value), "{}", "Next lookup on iterator with old position {old_key:?}, expecting one of {expected:?}, found {new_key_value:?}");
db.iter_current_key = Some(
new_key_value
.map_or(IterPosition::End, |(k, _)| IterPosition::Value(k[0])),
);
},
}
},
}
retry_operation(|| Self::check_db_and_model_are_equals(&db.db, &layers)).unwrap();
}
Expand Down
3 changes: 2 additions & 1 deletion src/btree/btree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use super::*;
use crate::{
btree::BTreeTable,
column::Column,
db::ValuePtr,
error::Result,
log::{LogQuery, LogWriter},
table::key::TableKeyQuery,
Expand Down Expand Up @@ -35,7 +36,7 @@ impl BTree {

pub fn write_sorted_changes(
&mut self,
mut changes: &[Operation<Vec<u8>, Vec<u8>>],
mut changes: &[Operation<ValuePtr, ValuePtr>],
btree: TablesRef,
log: &mut LogWriter,
) -> Result<()> {
Expand Down
48 changes: 29 additions & 19 deletions src/btree/iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
/// latest accessed key.u
use super::*;
use crate::{
btree::BTreeTable, db::CommitOverlay, error::Result, log::LogQuery, parking_lot::RwLock,
btree::BTreeTable,
db::{CommitOverlay, ValuePtr},
error::Result,
log::LogQuery,
parking_lot::RwLock,
table::key::TableKeyQuery,
};

Expand Down Expand Up @@ -41,7 +45,7 @@ pub struct BTreeIterator<'a> {
last_key: LastKey,
}

type IterResult = Result<Option<(Vec<u8>, Vec<u8>)>>;
type IterResult = Result<Option<(ValuePtr, ValuePtr)>>;

#[derive(Debug)]
struct PendingBackend {
Expand All @@ -53,8 +57,8 @@ struct PendingBackend {
pub enum LastKey {
Start,
End,
At(Vec<u8>),
Seeked(Vec<u8>),
At(ValuePtr),
Seeked(ValuePtr),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit hesitant to switch this one to rc as it changes the iterator api. Seeked is not really useful, At will be to save one key memory per iterator and a mem copy of the key on all alloc (could use an allocated buffer but not sure if relevant) but the iterator is nowhere near performant or trying to be.
Personally I would rather keep the rc to the change overlay only.

Copy link
Contributor Author

@hanabi1224 hanabi1224 Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you elaborate on which API to keep unchanged? I just tried reverting it here but it does not change any API signature. commit: 1af4855

Copy link
Collaborator

@cheme cheme Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just iterator next and prev returns IterResult that used to contain plain Vec (as used in the fuzzer, I mean the change to the fuzzer could also be reverted then).

Copy link
Contributor Author

@hanabi1224 hanabi1224 Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@cheme Done. Please take another look. (Some changes come from cargo clippy --fix and I didn't revert them)

}

#[derive(Debug)]
Expand Down Expand Up @@ -93,7 +97,7 @@ impl<'a> BTreeIterator<'a> {
// seek require log do not change
let log = self.log.read();
let record_id = log.last_record_id(self.col);
self.last_key = LastKey::Seeked(key.to_vec());
self.last_key = LastKey::Seeked(key.to_vec().into());
self.pending_backend = None;
self.seek_backend(SeekTo::Include(key), record_id, self.table, &*log)
}
Expand Down Expand Up @@ -146,8 +150,8 @@ impl<'a> BTreeIterator<'a> {
self.next_backend(record_id, self.table, &*log, direction)?
};
let result = match (next_commit_overlay, next_backend) {
(Some((commit_key, commit_value)), Some((backend_key, backend_value))) =>
match (direction, commit_key.cmp(&backend_key)) {
(Some((commit_key, commit_value)), Some((backend_key, backend_value))) => {
match (direction, commit_key.value().cmp(&backend_key)) {
(IterDirection::Backward, std::cmp::Ordering::Greater) |
(IterDirection::Forward, std::cmp::Ordering::Less) => {
self.pending_backend = Some(PendingBackend {
Expand All @@ -162,15 +166,17 @@ impl<'a> BTreeIterator<'a> {
}
},
(IterDirection::Backward, std::cmp::Ordering::Less) |
(IterDirection::Forward, std::cmp::Ordering::Greater) => Some((backend_key, backend_value)),
(IterDirection::Forward, std::cmp::Ordering::Greater) =>
Some((backend_key.into(), backend_value.into())),
(_, std::cmp::Ordering::Equal) =>
if let Some(value) = commit_value {
Some((backend_key, value))
Some((backend_key.into(), value))
} else {
self.last_key = LastKey::At(commit_key);
continue
},
},
}
},
(Some((commit_key, Some(commit_value))), None) => {
self.pending_backend = Some(PendingBackend { next_item: None, direction });
Some((commit_key, commit_value))
Expand All @@ -180,7 +186,8 @@ impl<'a> BTreeIterator<'a> {
self.last_key = LastKey::At(k);
continue
},
(None, Some((backend_key, backend_value))) => Some((backend_key, backend_value)),
(None, Some((backend_key, backend_value))) =>
Some((backend_key.into(), backend_value.into())),
(None, None) => {
self.pending_backend = Some(PendingBackend { next_item: None, direction });
None
Expand Down Expand Up @@ -214,10 +221,10 @@ impl<'a> BTreeIterator<'a> {
*tree = new_tree;
match &self.last_key {
LastKey::At(last_key) => {
iter.seek(SeekTo::Exclude(last_key.as_slice()), tree, col, log)?;
iter.seek(SeekTo::Exclude(last_key.value().as_slice()), tree, col, log)?;
},
LastKey::Seeked(last_key) => {
iter.seek(SeekTo::Include(last_key.as_slice()), tree, col, log)?;
iter.seek(SeekTo::Include(last_key.value().as_slice()), tree, col, log)?;
},
LastKey::Start => {
iter.seek(SeekTo::Include(&[]), tree, col, log)?;
Expand Down Expand Up @@ -328,29 +335,32 @@ impl BTreeIterState {
(IterDirection::Forward, LastIndex::At(sep)) if is_leaf =>
LastIndex::At(*sep + 1),
(IterDirection::Forward, LastIndex::At(sep)) => LastIndex::Descend(*sep + 1),
(IterDirection::Forward, LastIndex::Before(sep)) if *sep == ORDER =>
(IterDirection::Forward, LastIndex::Before(sep)) if *sep == ORDER => {
if self.exit(direction) {
break
} else {
continue
},
}
},
(IterDirection::Forward, LastIndex::Before(sep)) => LastIndex::At(*sep),

(IterDirection::Backward, LastIndex::At(sep)) if is_leaf && *sep == 0 =>
(IterDirection::Backward, LastIndex::At(sep)) if is_leaf && *sep == 0 => {
if self.exit(direction) {
break
} else {
continue
},
}
},
(IterDirection::Backward, LastIndex::At(sep)) if is_leaf =>
LastIndex::At(*sep - 1),
(IterDirection::Backward, LastIndex::At(sep)) => LastIndex::Descend(*sep),
(IterDirection::Backward, LastIndex::Before(sep)) if *sep == 0 =>
(IterDirection::Backward, LastIndex::Before(sep)) if *sep == 0 => {
if self.exit(direction) {
break
} else {
continue
},
}
},
(IterDirection::Backward, LastIndex::Before(sep)) => LastIndex::At(*sep - 1),
};
match next {
Expand Down
18 changes: 11 additions & 7 deletions src/btree/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,24 +357,28 @@ pub mod commit_overlay {
use super::*;
use crate::{
column::{ColId, Column},
db::{BTreeCommitOverlay, Operation},
db::{BTreeCommitOverlay, Operation, ValuePtr},
error::Result,
};

#[derive(Debug)]
pub struct BTreeChangeSet {
pub col: ColId,
pub changes: Vec<Operation<Vec<u8>, Vec<u8>>>,
pub changes: Vec<Operation<ValuePtr, ValuePtr>>,
}

impl BTreeChangeSet {
pub fn new(col: ColId) -> Self {
BTreeChangeSet { col, changes: Default::default() }
}

pub fn push(&mut self, change: Operation<Vec<u8>, Vec<u8>>) {
pub fn push(&mut self, change: Operation<Value, Value>) {
// No key hashing
self.changes.push(change);
self.changes.push(match change {
Operation::Set(k, v) => Operation::Set(k.into(), v.into()),
Operation::Dereference(k) => Operation::Dereference(k.into()),
Operation::Reference(k) => Operation::Reference(k.into()),
});
}

pub fn copy_to_overlay(
Expand All @@ -388,16 +392,16 @@ pub mod commit_overlay {
for change in self.changes.iter() {
match change {
Operation::Set(key, value) => {
*bytes += key.len();
*bytes += value.len();
*bytes += key.value().len();
*bytes += value.value().len();
overlay.insert(key.clone(), (record_id, Some(value.clone())));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key and value are Arc<Vec<u8>> now

},
Operation::Dereference(key) => {
// Don't add removed ref-counted values to overlay.
// (current ref_counted implementation does not
// make much sense for btree indexed content).
if !ref_counted {
*bytes += key.len();
*bytes += key.value().len();
overlay.insert(key.clone(), (record_id, None));
}
},
Expand Down
17 changes: 9 additions & 8 deletions src/btree/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use super::{
};
use crate::{
column::Column,
db::ValuePtr,
error::Result,
index::Address,
log::{LogQuery, LogWriter},
Expand Down Expand Up @@ -55,7 +56,7 @@ impl Node {
&mut self,
parent: Option<(&mut Self, usize)>,
depth: u32,
changes: &mut &[Operation<Vec<u8>, Vec<u8>>],
changes: &mut &[Operation<ValuePtr, ValuePtr>],
btree: TablesRef,
log: &mut LogWriter,
) -> Result<(Option<(Separator, Child)>, bool)> {
Expand All @@ -70,7 +71,7 @@ impl Node {
}
let r = match &changes[0] {
Operation::Set(key, value) =>
self.insert(depth, key, value, changes, btree, log)?,
self.insert(depth, key.value(), value.value(), changes, btree, log)?,
_ => self.on_existing(depth, changes, btree, log)?,
};
if r.0.is_some() || r.1 {
Expand All @@ -81,12 +82,12 @@ impl Node {
}
if let Some((parent, p)) = &parent {
let key = &changes[1].key();
let (at, i) = self.position(key)?; // TODO could start position from current
let (at, i) = self.position(key.value())?; // TODO could start position from current
if at || i < self.number_separator() {
*changes = &changes[1..];
continue
}
let (at, i) = parent.position(key)?;
let (at, i) = parent.position(key.value())?;
if !at && &i == p && i < parent.number_separator() {
*changes = &changes[1..];
continue
Expand All @@ -105,7 +106,7 @@ impl Node {
depth: u32,
key: &[u8],
value: &[u8],
changes: &mut &[Operation<Vec<u8>, Vec<u8>>],
changes: &mut &[Operation<ValuePtr, ValuePtr>],
btree: TablesRef,
log: &mut LogWriter,
) -> Result<(Option<(Separator, Child)>, bool)> {
Expand Down Expand Up @@ -235,18 +236,18 @@ impl Node {
fn on_existing(
&mut self,
depth: u32,
changes: &mut &[Operation<Vec<u8>, Vec<u8>>],
changes: &mut &[Operation<ValuePtr, ValuePtr>],
values: TablesRef,
log: &mut LogWriter,
) -> Result<(Option<(Separator, Child)>, bool)> {
let change = &changes[0];
let key = change.key();
let has_child = depth != 0;
let (at, i) = self.position(key)?;
let (at, i) = self.position(key.value())?;
if at {
let existing = self.separator_address(i);
if let Some(existing) = existing {
if Column::write_existing_value_plan::<_, Vec<u8>>(
if Column::write_existing_value_plan::<_, ValuePtr>(
&TableKey::NoHash,
values,
existing,
Expand Down
Loading