trie -> kv store #677
trie -> kv store #677
Conversation
arnetheduck
commented
Jan 15, 2020
arnetheduck
commented
- simplify data storage to key-value, tries are not relevant for NBC
- locked-down version of lmdb dependency
- easier to build / maintain on various platforms
|
Abstractly at least, I'm in favor of this, as RocksDB is by a wide margin the bottlenecking factor in a from-scratch build -- I guess the argument for RocksDB, despite its portability, build time and/or CI cache complexity costs, etc has been performance. How is that affected here? |
|
building there are downsides to lmdb as well - memory-mapping is not available everywhere, and has limitations on 32-bit platforms. that said, I'd consider this a stop-gap solution until we implement a hot/cold store where the cold store goes into a flat append-only file - at that point, the database will be so small that whatever advanced optimizations big databases like rocksdb do won't matter. the main feature of this branch is to simplify the storage model to KV instead of Trie - that's a better starting point for future storage work - in fact, it would make sense to rework nim-eth to use this virtual KV interface for its trie store, just like one could imagine extracting the lmdb parts into a separate library etc. that's a bridge for another day though. |
|
You need to also check for |
|
it's only w64 failing with the compile error though, 32-bit is fine because it builds beacon_node, except for the timeout which is strange... |
No. The 64-bit Windows build passes.
No, it's not and it doesn't. Remember what I said about having 2 parallel Make jobs? One hangs and the rest run to completion. The one that hangs is "beacon_node". I found out by running Hence the solution I gave you 13 hours ago: #677 (comment) |
|
well, one step forwards - one step backwards - got a bit further with the fix though it's a mystery why it doesn't stop the build then.. ? but now it crashes on some null pointer which I guess happens when loading that symbol which kind of sucks - any other tricks up your sleeve, @stefantalpalaru ? |
arnetheduck
force-pushed
the
lmdb
branch
2 times, most recently
from
363e51d
to
bc569c0
Compare
the symbols are loaded at runtime, so no linking needed: https://github.com/LMDB/lmdb/blob/mdb.master/libraries/liblmdb/mdb.c#L4744 |
Old Nim bug: nim-lang/Nim#8648
Just some uint64->uint changes, the usual csize->uint (until csize_t becomes available) and a very interesting stack corruption. It turns out that when you declare a C function with just an {.importc.} and no header, Nim gives it the "nimcall" calling convention, which is usually "fastcall". What you actually want is "cdecl" (same as "stdcall"). The former convention uses registers to pass parameters, the latter doesn't, on 32-bit: https://en.wikipedia.org/wiki/X86_calling_conventions#List_of_x86_calling_conventions |
upstream bug? |
* simplify data storage to key-value, tries are not relevant for NBC * locked-down version of lmdb dependency * easier to build / maintain on various platforms
|
|
Fixed. |
| @@ -0,0 +1,94 @@ | |||
| # Simple Key-Value store database interface | |||
There was a problem hiding this comment.
TrieDatabaseRef may have been named inappropriately, but it's already a simple key-value store. The only difference is that it comes with an additional support for discardable in-memory transactions, which doesn't complicate the API at all when it's not used.
I assume the get proc signature was changed as an optimisation (to avoid copying the result bytes to a newly allocated sequence). This optimisation could have been added to TrieDatabaseRef as well.
There was a problem hiding this comment.
it also has logic for maintaining an empty rlp item (thus pulling in the RLP code) and does refcounting in memory, none of which are needed here - this PR is about eliminating surface area for bugs by making sure the code does what it needs to do in the simplest way possible.
get changed for that reason, but there's a semantic change as well which in theory allows for empty values to happen. I do not have enough confidence in the test suite to perform this change on nim-eth itself - that can easily be done by someone more familiar with that code if need be, but with that said nim-beacon-chain and nimbus1 have completely different storage needs - they are likely to continue using different storage engines..
There was a problem hiding this comment.
fwiw, a practical outcome of the complexity of TrieDatabaseRef is that every implementation of it is faulty in the face of exceptions: leaks, reader/writer transaction blocking - the code raises exceptions but is not written for exception safety
