Reduce alloc in copy_to_overlay #178

hanabi1224 · 2023-02-03T19:43:34Z

I notice that the copy_to_overlay method clones the value vector in Hash index mode and clones both the key and value vectors in Btree index mode, which seems to be an unnecessary overhead, this PR tries to, instead, clone the pointer type Arc<Vec<u8>> which is much cheaper, please let me know your feedback, thanks!

hanabi1224 · 2023-02-03T19:44:40Z

src/btree/mod.rs

-						*bytes += key.len();
-						*bytes += value.len();
+						*bytes += key.value().len();
+						*bytes += value.value().len();
 						overlay.insert(key.clone(), (record_id, Some(value.clone())));


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

hanabi1224 · 2023-02-03T19:48:03Z

src/db.rs

@@ -1313,7 +1364,7 @@ impl IndexedChangeSet {
 			match &change {
 				Operation::Set(k, v) => {
 					*bytes += k.len();
-					*bytes += v.len();
+					*bytes += v.value().len();
 					overlay.indexed.insert(*k, (record_id, Some(v.clone())));


value is Arc<Vec<u8>> now

arkpar · 2023-02-04T07:51:59Z

Thank you for the PR.
I've ran the stress test with it and this does not seem to affect performance. Memory usage is sligtly lower indeed.

arkpar · 2023-02-04T07:55:22Z

src/db.rs

@@ -56,6 +57,47 @@ const KEEP_LOGS: usize = 16;
 /// Value is just a vector of bytes. Value sizes up to 4Gb are allowed.
 pub type Value = Vec<u8>;

+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
+pub struct ValuePtr(Arc<Value>);


Since this is used for both the key and the value, it should be named in a more generic way. Something like SharedVec or ArcVec or ArcBuf

Tpt

Thank you for this nice change! I just share the same naming concern as @arkpar

src/db.rs

cheme

Thanks, it sounds like a good idea to switch to Rc in the change overlay.

cheme · 2023-02-04T10:20:05Z

src/db.rs

@@ -56,6 +57,47 @@ const KEEP_LOGS: usize = 16;
 /// Value is just a vector of bytes. Value sizes up to 4Gb are allowed.
 pub type Value = Vec<u8>;

+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
+pub struct ValuePtr(Arc<Value>);


I would also maybe just consider a type alias like type RcKey = Arc<Key>; type RcValue = Arc<Value> to reduce the boilerplate implementation.

Renaming done

cheme · 2023-02-04T10:20:52Z

src/db.rs

+	}
+}
+
+impl<const N: usize> TryFrom<ValuePtr> for [u8; N] {


is this function used?

It's used in tests, let me mark it as #[cfg(test)]

cheme · 2023-02-04T10:37:39Z

src/btree/iter.rs

-	At(Vec<u8>),
-	Seeked(Vec<u8>),
+	At(ValuePtr),
+	Seeked(ValuePtr),


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.

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

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).

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

hanabi1224 · 2023-02-04T14:00:32Z

Thank you for the PR. I've ran the stress test with it and this does not seem to affect performance. Memory usage is sligtly lower indeed.

Where can I find the stress test code? In my scenario, I have ~70M pairs to inject upfront so it's a write-only scenario, and I have to commit batches at 1GB or 2GB to work around the hard-coded 16MB write queue size (This reduces time cost by ~50%). This change does help my scenario a bit

cheme · 2023-02-04T14:02:24Z

Thank you for the PR. I've ran the stress test with it and this does not seem to affect performance. Memory usage is sligtly lower indeed.

Where can I find the stress test code? In my scenario, I have ~70M pairs to ingest upfront so it's a write-only scenario, and I have to commit batches at 1GB or 2GB to work around the hard-coded 16MB write queue size. This change does help my scenario a bit

cargo run -p parity-db-admin -- stress --help

hanabi1224 · 2023-02-04T14:08:14Z

@cheme thanks!

cheme

Works for me.

hanabi1224 added 2 commits February 4, 2023 01:22

fix: reduce allocations in copy_to_overlay

43331db

btree copy_to_overlay

86ffde6

hanabi1224 commented Feb 3, 2023

View reviewed changes

hanabi1224 force-pushed the reduce_alloc_in_copy_to_overlay branch from c8c585c to 095fc7b Compare February 3, 2023 22:46

fix fmt and clippy

6e7ffb6

hanabi1224 force-pushed the reduce_alloc_in_copy_to_overlay branch from 095fc7b to 6e7ffb6 Compare February 3, 2023 22:48

arkpar requested review from Tpt and cheme February 4, 2023 07:52

arkpar reviewed Feb 4, 2023

View reviewed changes

Tpt reviewed Feb 4, 2023

View reviewed changes

src/db.rs Outdated Show resolved Hide resolved

cheme reviewed Feb 4, 2023

View reviewed changes

hanabi1224 added 4 commits February 4, 2023 20:43

rename

ca9f8d7

mark TryFrom as #[cfg(test)]

59bcdde

revert LastKey

1af4855

revert iter API change

3d40344

cheme approved these changes Feb 4, 2023

View reviewed changes

Tpt approved these changes Feb 4, 2023

View reviewed changes

arkpar merged commit b4af249 into paritytech:master Feb 6, 2023

hanabi1224 deleted the reduce_alloc_in_copy_to_overlay branch February 6, 2023 12:10

hanabi1224 mentioned this pull request Mar 21, 2023

chore: upgrade paritydb to 0.4.6 ChainSafe/forest#2692

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce alloc in copy_to_overlay #178

Reduce alloc in copy_to_overlay #178

hanabi1224 commented Feb 3, 2023 •

edited

Loading

hanabi1224 Feb 3, 2023

hanabi1224 Feb 3, 2023

arkpar commented Feb 4, 2023 •

edited

Loading

arkpar Feb 4, 2023

Tpt left a comment

cheme left a comment

cheme Feb 4, 2023

hanabi1224 Feb 4, 2023

cheme Feb 4, 2023

hanabi1224 Feb 4, 2023

cheme Feb 4, 2023

hanabi1224 Feb 4, 2023 •

edited

Loading

cheme Feb 4, 2023 •

edited

Loading

hanabi1224 Feb 4, 2023 •

edited

Loading

hanabi1224 commented Feb 4, 2023 •

edited

Loading

cheme commented Feb 4, 2023

hanabi1224 commented Feb 4, 2023

cheme left a comment

Reduce alloc in copy_to_overlay #178

Reduce alloc in copy_to_overlay #178

Conversation

hanabi1224 commented Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

Tpt left a comment

Choose a reason for hiding this comment

cheme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

cheme Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

hanabi1224 Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

hanabi1224 commented Feb 4, 2023 • edited Loading

cheme commented Feb 4, 2023

hanabi1224 commented Feb 4, 2023

cheme left a comment

Choose a reason for hiding this comment

hanabi1224 commented Feb 3, 2023 •

edited

Loading

arkpar commented Feb 4, 2023 •

edited

Loading

hanabi1224 Feb 4, 2023 •

edited

Loading

cheme Feb 4, 2023 •

edited

Loading

hanabi1224 Feb 4, 2023 •

edited

Loading

hanabi1224 commented Feb 4, 2023 •

edited

Loading