kvdb-rocksdb: optimize and rename iter_from_prefix #365
Conversation
| @@ -28,6 +27,11 @@ pub type KeyValuePair = (Box<[u8]>, Box<[u8]>); | |||
| /// Iterator with built-in synchronization. | |||
| pub struct ReadGuardedIterator<'a, I, T> { | |||
| inner: OwningHandle<UnsafeStableAddress<'a, Option<T>>, DerefWrapper<Option<I>>>, | |||
| // We store the upper bound here | |||
| // to make sure it lives at least as long as the iterator. | |||
| // See https://github.com/rust-rocksdb/rust-rocksdb/pull/309. | |||
There was a problem hiding this comment.
Also see rust-rocksdb/rust-rocksdb#377
| read_lock: RwLockReadGuard<'a, Option<T>>, | ||
| col: u32, | ||
| prefix: &[u8], | ||
| upper_bound: Box<[u8]>, |
There was a problem hiding this comment.
Small nit, I would have put the calculation to end_prefix (or the call to a function doing the calculation) inside this function to avoid this additional parameter in the api.
There was a problem hiding this comment.
I agree the current internal API is cumbersome, but we also pass here &ReadOptions, so in order to move the end_prefix calculation there, a bigger refactoring is needed. I'll leave it as is for now.
|
The parity-common/kvdb-rocksdb/src/lib.rs Line 526 in dd89c9a Putting on ice till I figure this out. |
| // rocksdb doesn't work with an empty upper bound | ||
| if !end_prefix.is_empty() { |
There was a problem hiding this comment.
cc @cheme, we might want to re-review prefix deletion code as rocksdb doesn't iterate at all if the upper bound is empty as it matches all keys
There was a problem hiding this comment.
I should have remember that, there was some similar things in the deletion prefix pr, let me check back:
parity-common/kvdb-rocksdb/src/lib.rs
Line 447 in dd89c9a
Is there a case where we got a empty upper bound and the start bound is not empty, ok [255] or [255, 255] ... So yes the test should be rewrite on the result of end prefix instead of the prefix 👍 I am doing a quick PR.
* 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) ...
We wanted to rename
iter_from_prefixfor a long time as it was a source of confusion. Hopefully, the new nameiter_with_prefixis less confusing :) Another alternative isiter_by_prefix.Also for RocksDB impl we're setting upper bound to optimize the prefix iteration.