[kvdb-rocksdb] switch to upstream #257

ordian · 2019-11-10T14:32:45Z

~~This is a draft PR until the following PRs are merged/issues are resolved~~:

Also (could be done in #256)

merge conflicts need to be resolved with kvdb-rocksdb: configurable memory budget per column #256 and kvdb-rocksdb: configurable memory budget per column #256 (comment).

Closes #88.

* master: add CONTRIBUTING guidelines and initial changelogs (#249) impl-serde: bump to 0.2.3 (#254) Fix `impl-serde::serializa/_raw` for empty slices (#253)

@ordian

…by way of paritytech/parity-common#257 by @ordian.

ordian · 2019-11-14T15:56:27Z

kvdb-rocksdb/src/lib.rs

-				let overlay = &self.overlay.read()[Self::to_overlay_column(col)];
+	/// Will hold a lock until the iterator is dropped
+	/// preventing the database from being closed.
+	pub fn iter<'a>(&'a self, col: Option<u32>) -> impl Iterator<Item = KeyValuePair> + 'a {


this is a breaking change (return type)

ordian · 2019-11-14T15:57:12Z

kvdb-rocksdb/src/lib.rs

@@ -531,26 +513,18 @@ impl Database {
 	/// Get value by partial key. Prefix size should match configured prefix size. Only searches flushed values.
 	// TODO: support prefix seek for unflushed data
 	pub fn get_by_prefix(&self, col: Option<u32>, prefix: &[u8]) -> Option<Box<[u8]>> {
-		self.iter_from_prefix(col, prefix).and_then(|mut iter| {
-			match iter.next() {
-				// TODO: use prefix_same_as_start read option (not available in C API currently)


we're finally using prefix_same_as_start

ordian · 2019-11-14T15:58:01Z

kvdb-rocksdb/src/lib.rs

+		block_opts.set_lru_cache(cache_size);
+		block_opts.set_cache_index_and_filter_blocks(true);
+		block_opts.set_pin_l0_filter_and_index_blocks_in_cache(true);
+		block_opts.set_bloom_filter(10, true);


this option is added based on https://github.com/paritytech/parity-substrate-rocksdb-tuning

ordian · 2019-11-14T16:15:32Z

kvdb-rocksdb/src/lib.rs

-	_marker: PhantomData<&'a Database>,
+struct DBAndColumns {
+	db: DB,
+	column_names: Vec<String>,


we have to store column names instead of columns, because cf_handle returns a reference to ColumnFamily.

ordian · 2019-11-14T16:16:27Z

kvdb-rocksdb/src/lib.rs

@@ -189,58 +189,37 @@ impl Default for DatabaseConfig {
 	}
 }

-/// Database iterator (for flushed data only)
-// The compromise of holding only a virtual borrow vs. holding a lock on the
-// inner DB (to prevent closing via restoration) may be re-evaluated in the future.


we now hold a (read) lock to prevent closing via restoration during iteration

ordian · 2019-11-14T16:25:55Z

kvdb-rocksdb/src/lib.rs

+		let read_lock = self.db.read();
+		let optional = if read_lock.is_some() {
+			let guarded = iter::ReadGuardedIterator::new_from_prefix(read_lock, col, prefix);
+			Some(interleave_ordered(Vec::new(), guarded))


I wonder whether we should search in overlay column as we do in iter (cc @arkpar)?

* master: Bump rlp crate version. (#270) Introduce Rlp::at_with_offset method. (#269) Make fixed-hash test structs public (#267) Migrate primitive types to 2018 edition (#262) upgrade tiny-keccak to 2.0 (#260)

kvdb-rocksdb/src/iter.rs

kvdb-rocksdb/src/lib.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

kvdb-rocksdb/Cargo.toml

bkchr · 2019-11-27T09:52:24Z

Changing the block size seems to have fixed our problems in Polkadot :)

dvdplm

lgtm modulo lz4

kvdb-rocksdb/Cargo.toml

* master: travis: try to fix wasmpack chrome test on macOS (#263) Use 2018 edition for rustfmt (#266) [fixed-hash]: re-export `alloc_` (#268) kvdb-web: async-awaitify (#259)

bkchr

Mostly okay, just some nitpicks.

bkchr · 2019-11-27T10:11:11Z

kvdb-rocksdb/src/lib.rs

@@ -672,17 +643,16 @@ impl Database {
 		self.db
 			.read()
 			.as_ref()
-			.and_then(|db| if db.cfs.is_empty() { None } else { Some(db.cfs.len()) })
+			.and_then(|db| if db.column_names.is_empty() { None } else { Some(db.column_names.len()) })
 			.map(|n| n as u32)
 			.unwrap_or(0)
 	}

 	/// Drop a column family.
 	pub fn drop_column(&self) -> io::Result<()> {


As we make a breaking release anyway, shouldn't this be called pop_column()?

Should we rename add_column to push_column as well?

Yeah, good idea :)

to my ears, add_column sounds better and is easier to understand. Using push/pop sort of implies something temporary or something done fairly frequently (which adding/removing columns is not).
I'd keep add_column and maaaybe rename drop_column to remove_column.

I just think that drop_column or remove_column sounds like something that gets an index and removes the column at the given index and not removes the last column.

That is a good point. How about append_column() and remove_last_column()?

As there is no consensus on naming, let's keep it as is, we're planning to release a new breaking version with format_version, etc after that anyway.

bkchr · 2019-11-27T10:35:52Z

kvdb-rocksdb/src/iter.rs

+	}
+}
+
+pub trait IterationHandler {


Some documentation would be nice :)

This trait was originally added in #120, we can remove it and all generics here, but it's good to have if we will consider merging something similar to #120.

Another idea I had is replace RwLock<Option<DBAndColumns>> with https://docs.rs/arc-swap/0.4.4/arc_swap/, but let's keep it as is for now.

kvdb-rocksdb/src/iter.rs

…h/parity-common into ao-rocksdb-switch-to-upstream2 * 'ao-rocksdb-switch-to-upstream2' of github.com:paritytech/parity-common: kvdb-rocksdb: remove lz4 feature as it has no effect for now travis: try to fix wasmpack chrome test on macOS (#263) kvdb-rocksdb: please the CI kvdb-rocksdb: do not account for default column memory budget Use 2018 edition for rustfmt (#266) [fixed-hash]: re-export `alloc_` (#268) kvdb-web: async-awaitify (#259)

…h/parity-common into ao-rocksdb-switch-to-upstream2 * 'ao-rocksdb-switch-to-upstream2' of github.com:paritytech/parity-common: kvdb-rocksdb: add a workaround for the rocksdb prefix bug

@ordian

* Use upstream rocksdb …by way of paritytech/parity-common#257 by @ordian. * Hint at how `parity db reset` works in the error message * migration-rocksdb: fix build * Cargo.toml: use git dependency instead of path * update to latest kvdb-rocksdb * fix tests * saner default for light client * rename open_db to open_db_light * update to latest kvdb-rocksdb * moar update to latest kvdb-rocksdb * even moar update to latest kvdb-rocksdb * use kvdb-rocksdb from crates.io * Update parity/db/rocksdb/helpers.rs * add docs to memory_budget division

sherlock-shi-x · 2019-12-10T06:34:22Z

We use parity as node for pool mining, so the performance is the major factor.

We use 12c/96g/400g ssd machine and parity v2.5.11 to sync blocks, the distribution of cache is similar to default 7:2:1.

When we trace the cache of rocksdb, we find the pr #256 and #257, and find the block_size 16KB in rocksdb-0.1.6

	pub fn ssd() -> CompactionProfile {
		CompactionProfile {
			initial_file_size: 64 * MB as u64,
			block_size: 16 * KB,
			write_rate_limit: None,
		}
	}

However in rocksdb-0.2.0, @grbIzl tried to optimize block_size to 8MB but was rollback to 16KB in this pr.

We want to know if it is suitable for us(mining) to set block_size 8MB and any other advice for
parity (cache) configuration due to our machine, thank you.
@ordian @grbIzl

* master: Compile triehash for no_std (#280) [kvdb-rocksdb] Use "pinned" gets to avoid allocations (#274) [kvdb-rocksdb] Release 0.2 (#273) [kvdb-rocksdb] switch to upstream (#257) travis: try to fix wasmpack chrome test on macOS (#263) Use 2018 edition for rustfmt (#266) [fixed-hash]: re-export `alloc_` (#268) kvdb-web: async-awaitify (#259) kvdb-rocksdb: configurable memory budget per column (#256) Bump rlp crate version. (#270) Introduce Rlp::at_with_offset method. (#269) Make fixed-hash test structs public (#267) Migrate primitive types to 2018 edition (#262) upgrade tiny-keccak to 2.0 (#260)

ordian · 2019-12-10T08:37:56Z

@haihongS increasing block_size to 8mb introduced a performance regression, so we changed the value back. Increasing block_size increases read amplification and is recommended to be 16-64 kb. It's possible to set it to 8mb and drastically increase the cache size, but there is no perf benefit in that.

sherlock-shi-x · 2019-12-11T02:01:26Z

@haihongS increasing block_size to 8mb introduced a performance regression, so we changed the value back. Increasing block_size increases read amplification and is recommended to be 16-64 kb. It's possible to set it to 8mb and drastically increase the cache size, but there is no perf benefit in that.

@ordian, thanks for your patience, we have changed some codes in v2.5.11 and exposed the rocksdb statistics(ffi) in order to analyse the performance of rocksdb. I am very curious about the way of perf analyse in official parity group, can you offer any tips?

@ordian

* Use upstream rocksdb …by way of paritytech/parity-common#257 by @ordian. * Hint at how `parity db reset` works in the error message * migration-rocksdb: fix build * Cargo.toml: use git dependency instead of path * update to latest kvdb-rocksdb * fix tests * saner default for light client * rename open_db to open_db_light * update to latest kvdb-rocksdb * moar update to latest kvdb-rocksdb * even moar update to latest kvdb-rocksdb * use kvdb-rocksdb from crates.io * Update parity/db/rocksdb/helpers.rs * add docs to memory_budget division

bkchr and others added 7 commits October 28, 2019 18:04

Switch from parity-rocksdb to upstream rust-rocksdb

215a28d

wip

2c0468f

wip

d445cde

Merge branch 'master' into ao-rocksdb-switch-to-upstream2

2d42479

* master: add CONTRIBUTING guidelines and initial changelogs (#249) impl-serde: bump to 0.2.3 (#254) Fix `impl-serde::serializa/_raw` for empty slices (#253)

kvdb-rocksdb: working iterator

47a6e22

kvdb-rocksdb: cleanup

81bce7c

kvdb-rocksdb: more cleanup

71b2af0

dvdplm added a commit to openethereum/parity-ethereum that referenced this pull request Nov 11, 2019

Use upstream rocksdb

a398ee5

…by way of paritytech/parity-common#257 by @ordian.

dvdplm mentioned this pull request Nov 11, 2019

Use upstream rocksdb openethereum/parity-ethereum#11248

Merged

ordian added 4 commits November 12, 2019 10:29

kvdb-rocksdb: use options from updated upstream

e5cd5cb

kvdb-rocksdb: set bloom filter as recommended by tuning guide

c7640d1

kvdb-rocksdb: fix build

fb208ad

kvdb-rocksdb: set_level_compaction_dynamic_level_bytes

8a2442a

ordian mentioned this pull request Nov 12, 2019

kvdb-rocksdb: configurable memory budget per column #256

Merged

kvdb-rocksdb: switch to just published version

fa16fb5

ordian marked this pull request as ready for review November 14, 2019 15:55

ordian added the A0-pleasereview label Nov 14, 2019

ordian added 2 commits November 14, 2019 17:21

kvdb-rocksdb: preserve the old compression_per_level setting

3b31e4f

kvdb-rocksdb: add some iter module docs

f595233

ordian commented Nov 14, 2019

View reviewed changes

ordian added M5-dependencies M4-core labels Nov 14, 2019

ordian commented Nov 14, 2019

View reviewed changes

ordian and others added 5 commits November 19, 2019 10:24

kvdb-rocksdb: remove path on kvdb dependency temporarily

df8d614

kvdb-rocksdb: use only lz4 and snappy features

a479485

Merge branch 'master' into ao-rocksdb-switch-to-upstream2

1e1a865

* master: Bump rlp crate version. (#270) Introduce Rlp::at_with_offset method. (#269) Make fixed-hash test structs public (#267) Migrate primitive types to 2018 edition (#262) upgrade tiny-keccak to 2.0 (#260)

kvdb-rocksdb: support zstd compression as well

3153cca

Also add kvdb as path dependency

ed372f4

bkchr reviewed Nov 22, 2019

View reviewed changes

Apply suggestions from code review

28de430

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Add tests for budget calculation

3a071af

dvdplm reviewed Nov 26, 2019

View reviewed changes

kvdb-rocksdb/Cargo.toml Outdated Show resolved Hide resolved

dvdplm and others added 3 commits November 27, 2019 12:17

Add test to check the rocksdb settings

cd343cc

kvdb-rocksdb: do not account for default column memory budget

7eeaf7b

kvdb-rocksdb: please the CI

bd5abf1

ordian requested a review from dvdplm November 27, 2019 14:37

dvdplm approved these changes Nov 27, 2019

View reviewed changes

kvdb-rocksdb/Cargo.toml Outdated Show resolved Hide resolved

ordian added 2 commits November 27, 2019 16:17

kvdb-rocksdb: remove lz4 feature as it has no effect for now

f23574f

Merge branch 'master' into ao-rocksdb-switch-to-upstream2

8986947

* master: travis: try to fix wasmpack chrome test on macOS (#263) Use 2018 edition for rustfmt (#266) [fixed-hash]: re-export `alloc_` (#268) kvdb-web: async-awaitify (#259)

bkchr approved these changes Nov 27, 2019

View reviewed changes

dvdplm and others added 8 commits November 28, 2019 08:36

Address review grumbles

6a64e36

kvdb-rocksdb: add failing iter_from_prefix test

0217438

kvdb-rocksdb: add a workaround for the rocksdb prefix bug

e0d59e5

More prefix iter test

c4b5413

Merge branch 'ao-rocksdb-switch-to-upstream2' of github.com:paritytec…

adde7df

…h/parity-common into ao-rocksdb-switch-to-upstream2 * 'ao-rocksdb-switch-to-upstream2' of github.com:paritytech/parity-common: kvdb-rocksdb: add a workaround for the rocksdb prefix bug

Fix tests (hex, it's so hard)

b40b416

whitespace

38876cf

dvdplm merged commit 8fb8f13 into master Nov 28, 2019

dvdplm deleted the ao-rocksdb-switch-to-upstream2 branch November 28, 2019 13:21

ordian restored the ao-rocksdb-switch-to-upstream2 branch November 28, 2019 16:02

ordian mentioned this pull request Dec 4, 2019

kvdb-rocksdb: pass ReadOptions to iterators #277

Merged

bkchr deleted the ao-rocksdb-switch-to-upstream2 branch December 10, 2019 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kvdb-rocksdb] switch to upstream #257

[kvdb-rocksdb] switch to upstream #257

ordian commented Nov 10, 2019 •

edited

Loading

ordian Nov 14, 2019

ordian Nov 14, 2019

ordian Nov 14, 2019

ordian Nov 14, 2019

ordian Nov 14, 2019

ordian Nov 14, 2019

bkchr commented Nov 27, 2019

dvdplm left a comment

bkchr left a comment

bkchr Nov 27, 2019

ordian Nov 27, 2019

bkchr Nov 27, 2019

dvdplm Nov 27, 2019 •

edited

Loading

bkchr Nov 27, 2019

dvdplm Nov 27, 2019

ordian Nov 28, 2019

bkchr Nov 27, 2019

ordian Nov 28, 2019

sherlock-shi-x commented Dec 10, 2019

ordian commented Dec 10, 2019

sherlock-shi-x commented Dec 11, 2019

[kvdb-rocksdb] switch to upstream #257

[kvdb-rocksdb] switch to upstream #257

Conversation

ordian commented Nov 10, 2019 • edited Loading

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

bkchr commented Nov 27, 2019

dvdplm left a comment

Choose a reason for hiding this comment

bkchr 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

dvdplm Nov 27, 2019 • edited Loading

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

sherlock-shi-x commented Dec 10, 2019

ordian commented Dec 10, 2019

sherlock-shi-x commented Dec 11, 2019

ordian commented Nov 10, 2019 •

edited

Loading

dvdplm Nov 27, 2019 •

edited

Loading