Conversation
This reverts commit f9e431b.
* master: (284 commits) fix(serde deser): remove needless `len` check Update ci to crate removal. fix(deser): fix indexing into `str` bug Clean deprecated crate. Remove deps to hashdb, memdb, patricia-trie, keccak-hash. add uint operator benchmarks (#82) Update repo url Add issue number Fix ethereum-types tests readme: remove broken appveyor badge Update parity-crypto to ring v0.14 (#99) update impl-codec and update primitive-types (#101) primitive types new release Use TryFrom trait Use local path deps Deal with remaining merge issues update parity-codec for primitive types (#98) Update uint to 0.6 Add more conversion functions for U128 typo: duplicate features key ...
* master: Fix impl-rlp for uint in primitive-types (#117)
|
|
||
| fn iter(&self, col: usize) -> Self::Iterator { | ||
| // TODO: how to handle errors properly? | ||
| let ro_txn = self.ro_txn().expect("lmdb: transaction creation failed"); |
There was a problem hiding this comment.
Read-only transaction creation can fail if too many thread are trying to create a read-only transaction or we're out of memory (cf. http://www.lmdb.tech/doc/group__mdb.html#gad7ea55da06b77513609efebd44b26920)
| /// Get value by partial key. Prefix size should match configured prefix size. Only searches flushed values. | ||
| pub fn get_by_prefix(&self, col: Option<u32>, prefix: &[u8]) -> Option<Box<[u8]>> { | ||
| match self.iter_from_prefix(col, prefix).next() { | ||
| Some((k, v)) => if k.starts_with(prefix) { Some(v) } else { None }, |
There was a problem hiding this comment.
Why is this starts_with check necessary? Can LMDB return values that do not match the prefix?
Can't we do just:
match self.iter_from_prefix(col, prefix).next() {
Some((_, v)) => Some(v),
_ => None
}
or even self.iter_from_prefix(col, prefix).next().map(|(_,v)| v)?
There was a problem hiding this comment.
It is actually coming from rocksdb implementation (
parity-common/kvdb-rocksdb/src/lib.rs
Line 541 in 1409dda
There was a problem hiding this comment.
Interesting. I guess the same question applies! Doesn't rocksdb support prefix matching?
If it's a rocksdb quirk, maybe we can add a TODO to clean it up?
There was a problem hiding this comment.
There was a problem hiding this comment.
For posterity, this seems to be fixed in upstream rocksdb, from which we have diverged quite a bit I believe. Oddly enough the parity-rocksdb fork names prefix search as one of the reasons for forking, and yet I can't find it in our code.
Co-Authored-By: ordian <write@reusable.software>
Thanks David :) Co-Authored-By: ordian <write@reusable.software>
|
worth reading if we ever want to get back to this pr: |
|
we're already using zerocopy bindings to lmdb here, but https://github.com/Kerollmops/heed also worth considering if we're getting back to this |
|
Currently sled looks like a more promising approach and lmdb slow write speed is also something sled acknoledges https://github.com/spacejam/sled/wiki/motivating-experiences#lmdb |
|
Closing as stale. |
3 participants
On top of #120 (last commit only)