kvdb: no overlay #313

ordian · 2020-01-14T13:34:28Z

Fixes #310.

ordian · 2020-01-14T13:36:56Z

kvdb/src/lib.rs

-/// written can always be read, but may be present in an in-memory buffer. Values which have
-/// been flushed have been moved to backing storage, like a RocksDB instance. There are certain
-/// operations which are only guaranteed to operate on flushed data and not buffered,
-/// although implementations may differ in this regard.


Anyone who relies on this semantics shouldn't have done it on the first place, it is not written anywhere (is it?)

@NikVolf ^^^ :P

This is probably about iterators? :)

NikVolf · 2020-01-14T14:00:04Z

I wonder if we can get rid of locks on self.db, but it's a minor issue
probably required for safe closing/upgrading

ordian · 2020-01-14T14:02:31Z

I wonder if we can get rid of locks on self.db, but it's a minor issue
probably required for safe closing/upgrading

I considered using https://docs.rs/arc-swap/0.4.4/arc_swap/, but read locks are usually cheap enough, also not sure if holding a read lock during iteration to prevent the db from closing is a good idea in the end.

NikVolf · 2020-01-14T14:04:56Z

kvdb-rocksdb/src/lib.rs

 	pub fn get_by_prefix(&self, col: u32, prefix: &[u8]) -> Option<Box<[u8]>> {
 		self.iter_from_prefix(col, prefix).next().map(|(_, v)| v)
 	}

-	/// Get database iterator for flushed data.


it was probably lying before?
since it was interleaved with actual data?

partially, I guess
in order to support non-flushed data properly, it would have to take flushing into account (see e.g. get)

ordian · 2020-01-14T14:20:54Z

I wonder if we can get rid of locks on self.db, but it's a minor issue
probably required for safe closing/upgrading

I considered using https://docs.rs/arc-swap/0.4.4/arc_swap/, but read locks are usually cheap enough, also not sure if holding a read lock during iteration to prevent the db from closing is a good idea in the end.

Let's move this conversation to #314.

NikVolf · 2020-01-14T16:21:48Z

Great stuff!

dvdplm

Code looks great, a significant cleanup. I haven't read the audit yet so I'll be back with a proper review.

kvdb-rocksdb/src/lib.rs

dvdplm · 2020-01-14T20:55:40Z

kvdb-rocksdb/src/lib.rs

 	pub fn get_by_prefix(&self, col: u32, prefix: &[u8]) -> Option<Box<[u8]>> {
 		self.iter_from_prefix(col, prefix).next().map(|(_, v)| v)
 	}

-	/// Get database iterator for flushed data.
+	/// Get database iterator for the data.
 	/// Will hold a lock until the iterator is dropped
 	/// preventing the database from being closed.


I've probably asked this before, apologies, but why is it bad to close the DB while some thread is iterating over data? Is the assumption that threads iterating over some data must be allowed to complete?

this is a good question, but I consider it to be part of the #314

kvdb-rocksdb/src/lib.rs

kvdb/src/lib.rs

dvdplm · 2020-01-14T20:59:57Z

kvdb/src/lib.rs

@@ -118,25 +109,16 @@ pub trait KeyValueDB: Sync + Send + parity_util_mem::MallocSizeOf {
 	/// Get a value by key.
 	fn get(&self, col: u32, key: &[u8]) -> io::Result<Option<DBValue>>;

-	/// Get a value by partial key. Only works for flushed data.
+	/// Get a value by partial key. Only works for the data.


Suggested change

/// Get a value by partial key. Only works for the data.

/// Get the first value matching the given prefix.

Is it first or any random value?

First in the current implementation and I think we're relying on that, but maybe I'm wrong

ordian · 2020-01-14T23:04:48Z

kvdb/src/lib.rs

 	fn iter<'a>(&'a self, col: u32) -> Box<dyn Iterator<Item = (Box<[u8]>, Box<[u8]>)> + 'a>;

-	/// Iterate over flushed data for a given column, starting from a given prefix.
+	/// Iterate over the data for a given column, starting from a given prefix.
 	fn iter_from_prefix<'a>(


maybe we want to finally rename it, but let's do it in a separate PR

Co-Authored-By: David <[email protected]>

* master: keccak-hash: switch benches to criterion (#315)

dvdplm · 2020-02-07T11:24:53Z

So, any progress on this? I'd love to see some benchmarks on both eth and substrate before moving ahead.

* master: Add different mode for malloc_size_of_is_0 macro dealing with generics (#334) [parity-crypto] Use upstream secp256k1 (#258) Bump parking_lot to 0.10 and minor versions (#332) Remove libc completely (#333) update changelogs (#329) bump parity-util-mem to 0.4.2 (#328) remove libc feature from fixed-hash (#317) kvdb-rocksdb: release 0.4.2 (#327) kvdb-rocksdb: fix iter_from_prefix being slow (#326) MallocSizeOf for BTreeSet (#325) split off primitives (#323) travis: disable kvdb-web tests for chrome (#324) Expand const fn coverage (#319) uint: make zero const fn (#318) README: fix appveyor badge (#316)

ordian · 2020-02-07T11:32:56Z

I did import benchmarks on eth side #310 (comment), but we currently lack some read-heavy benches like RPC load etc. Would be nice if someone could bench it in substrate as well.

* master: prepare rlp-derive release (#344) Update/change licenses: MIT/Apache2.0 (#342) rlp-derive extracted (#343) Format for readme and changelog corrected (#341) Parity runtime moved to parity common for publication in crates.io (#271) Disable cache if explicit memory budget=0 passed (#339) [parity-crypto] prepare 0.5.0 (#336) [parity crypto]: remove unused depend `rustc_hex` (#337) Update doc comment (#335)

* master: kvdb-rocksdb: bump version (#348) kvdb-rocksdb: expose RocksDB stats (#347) Implement Error for FromDecStrErr (#346) Fix clippy lints for rlp-derive (#345)

* master: parity-util-mem: prepare release for 0.5.2 (#359) travis: test parity-util-mem on android (#358) parity-util-mem: update mimalloc feature (#352) kvdb: remove parity-bytes dependency (#351) parity-util-mem: use malloc for usable_size on android (#355) CI: troubleshoot macOS build (#356)

* master: Memtest example for Rocksdb (#349) Prep for release (#361)

ordian · 2020-03-26T11:41:24Z

Getting back to this, since substrate doesn't use write_buffered method at all, I don't expect this PR to have any effect on substrate. For eth, write_buffered only used in warp-sync, not regular import, so even if it will slow down warp-sync import by 5%, I think it's fine.

@dvdplm let me know what you think

dvdplm · 2020-03-26T12:39:13Z

For eth, write_buffered only used in warp-sync, not regular import, so even if it will slow down warp-sync import by 5%, I think it's fine.

rg "write_buffered\(" | wc -l shows 19 calls in eth. Many of them look like they are not perf sensitive at all, but commit_block() and feed() are pretty important calls to keep fast. A 5% pessimization is probably not ok there. :/

I really like this PR and I want it to land, but I still want to be convinced we have hard data to support the decision.

ordian · 2020-03-26T13:04:06Z

See #310 (comment) about eth usage of it.

dvdplm · 2020-03-26T14:18:27Z

See #310 (comment) about eth usage of it.

Oops, apologies. K, fine let's do this then. :)

* master: kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) Prep for release (#361)

* master: kvdb-rocksdb: optimize and rename iter_from_prefix (#365) bump parity-util-mem (#376) parity-util-mem: fix for windows (#375) keccak-hash: fix bench and add one for range (#372) [parity-crypto] Release 0.6.1 (#373) keccak-hash: bump version to 0.5.1 (#371) keccak-hash: add keccak256_range and keccak512_range functions (#370) Allow pubkey recovery for all-zero messages (#369) Delete by prefix operator in kvdb (#360) kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) Prep for release (#361) parity-util-mem: prepare release for 0.5.2 (#359) travis: test parity-util-mem on android (#358) parity-util-mem: update mimalloc feature (#352) kvdb: remove parity-bytes dependency (#351) parity-util-mem: use malloc for usable_size on android (#355) CI: troubleshoot macOS build (#356)

* master: (56 commits) primitive-types: add no_std support for serde feature (#385) Add Rocksdb Secondary Instance Api (#384) kvdb-rocksdb: update rocksdb to 0.14 (#379) prepare releases for a few crates (#382) uint: fix UB in uint::from_big_endian (#381) Fix limit prefix delete case (#368) Add arbitrary trait implementation (#378) kvdb-rocksdb: optimize and rename iter_from_prefix (#365) bump parity-util-mem (#376) parity-util-mem: fix for windows (#375) keccak-hash: fix bench and add one for range (#372) [parity-crypto] Release 0.6.1 (#373) keccak-hash: bump version to 0.5.1 (#371) keccak-hash: add keccak256_range and keccak512_range functions (#370) Allow pubkey recovery for all-zero messages (#369) Delete by prefix operator in kvdb (#360) kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) ...

ordian added 8 commits January 9, 2020 11:04

kvdb-rocksdb: no overlay experiment

9f79bf8

kvdb-rocksdb: remove unused dep

7e6f2ad

kvdb-rocksdb: panic on write failure

60329e7

kvdb: remove write_buffered and flush

b1defce

kvdb: update changelog

9c62d07

kvdb-rocksdb: update changelog

544ee1e

kvdb-web: update changelog

65635d1

kvdb-memory: update changelog

efb54b8

ordian commented Jan 14, 2020

View reviewed changes

ordian added the A0-pleasereview label Jan 14, 2020

ordian added 2 commits January 14, 2020 14:41

kvdb-rocksdb: fix the bench

7100682

kvdb-rocksdb: fix the bench #2

b82a5f4

NikVolf reviewed Jan 14, 2020

View reviewed changes

ordian mentioned this pull request Jan 14, 2020

kvdb-rocksdb: consider removing the RwLock #314

Closed

kvdb: cargo fmt

24eb74c

NikVolf approved these changes Jan 14, 2020

View reviewed changes

dvdplm reviewed Jan 14, 2020

View reviewed changes

dvdplm requested a review from arkpar January 14, 2020 21:02

ordian commented Jan 14, 2020

View reviewed changes

ordian and others added 3 commits January 15, 2020 00:09

Apply suggestions from code review

4ae7e3f

Co-Authored-By: David <[email protected]>

kvdb-rocksdb: s/acquired_val/value

2d2d670

Merge branch 'master' into ao-no-overlay

4842d63

* master: keccak-hash: switch benches to criterion (#315)

ordian added 2 commits February 7, 2020 12:29

kvdb-rocksdb: update the changelog

0784e54

ordian added 2 commits February 13, 2020 15:40

Merge branch 'master' into ao-no-overlay

20a62a4

* master: kvdb-rocksdb: bump version (#348) kvdb-rocksdb: expose RocksDB stats (#347) Implement Error for FromDecStrErr (#346) Fix clippy lints for rlp-derive (#345)

cheme mentioned this pull request Mar 13, 2020

Delete by prefix operator in kvdb #360

Merged

ordian added 2 commits March 16, 2020 10:51

Merge branch 'master' into ao-no-overlay

26ef26f

* master: Memtest example for Rocksdb (#349) Prep for release (#361)

dvdplm approved these changes Mar 26, 2020

View reviewed changes

dvdplm merged commit 7093b7e into master Mar 26, 2020

dvdplm deleted the ao-no-overlay branch March 26, 2020 14:18

ordian mentioned this pull request Aug 26, 2020

kvdb-rocksdb overlay cache is not effective paritytech/substrate#4508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvdb: no overlay #313

kvdb: no overlay #313

ordian commented Jan 14, 2020

ordian Jan 14, 2020

NikVolf Jan 14, 2020

NikVolf commented Jan 14, 2020

ordian commented Jan 14, 2020

NikVolf Jan 14, 2020

ordian Jan 14, 2020 •

edited

Loading

ordian commented Jan 14, 2020

NikVolf commented Jan 14, 2020

dvdplm left a comment

dvdplm Jan 14, 2020

ordian Jan 14, 2020

dvdplm Jan 14, 2020

dvdplm Jan 14, 2020

ordian Jan 14, 2020

ordian Jan 14, 2020

dvdplm commented Feb 7, 2020

ordian commented Feb 7, 2020

ordian commented Mar 26, 2020 •

edited

Loading

dvdplm commented Mar 26, 2020

ordian commented Mar 26, 2020

dvdplm commented Mar 26, 2020

	/// Get a value by partial key. Only works for the data.
	/// Get the first value matching the given prefix.

kvdb: no overlay #313

kvdb: no overlay #313

Conversation

ordian commented Jan 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikVolf commented Jan 14, 2020

ordian commented Jan 14, 2020

Choose a reason for hiding this comment

ordian Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

ordian commented Jan 14, 2020

NikVolf commented Jan 14, 2020

dvdplm 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

Choose a reason for hiding this comment

dvdplm commented Feb 7, 2020

ordian commented Feb 7, 2020

ordian commented Mar 26, 2020 • edited Loading

dvdplm commented Mar 26, 2020

ordian commented Mar 26, 2020

dvdplm commented Mar 26, 2020

ordian Jan 14, 2020 •

edited

Loading

ordian commented Mar 26, 2020 •

edited

Loading