[lmdb #3] Add LMDB storage backend #663
Conversation
meta-cla
bot
added
the
CLA Signed
✅ Deploy Preview for fb-oss-glean canceled.
|
simonmar
force-pushed
the
lmdb-3
branch
4 times, most recently
from
5ae6a5a to
2076683
Compare
|
Don't import this yet; I'm still fixing bugs. |
|
Should be good to go now, there will be some followups though. |
|
@simonhollis has imported this pull request. If you are a Meta employee, you can view this in D90599900. |
|
@simonmar Thanks for adding LMDB support. We're excited about the functionality. Is there a way to package the openldap code cleanly as a new third party dependency? It's preferable to not include that in directly into the glean file tree. |
Hi @simonhollis - sure I can do something. But are you sure you want it to be a separate dependency? It's only 4 files and bundling it directly is by far the simplest way. The license is in the source files too. We can't use the Linux package directly ( |
|
@simonhollis how about this? f64c145 |
|
I agree it's ok to not track the upstream repo. But I think the main point is we already have @simonmar IIUC this is very similar case for other "internally" available code? Can we do similar as what we do for folly? pulling sources at build time instead of putting the sources in the repo? Or maybe we could just keep it as in f64c145 for github repo and internally don't import these files but link to our pathes. @simonmar what you would suggest? |
There's also a small diff relative to upstream this is needed to get the increased key size. Perhaps we can get upstream to accept something to make this accessible with a I think it would be a shame to do something complex here because that negates the benefit of having a lightweight dependency, so:
that sounds good to me, if you can find some solution to the diff above. |
| v.packed(fact.clause.key_size); | ||
| v.put({fact.clause.data, fact.clause.size()}); | ||
|
|
||
| put(Family::entities, k.bytes(), v.bytes()); |
There was a problem hiding this comment.
I wonder what happens if the key is bigger? inconsistent behaviour? should we assert in that case?
There was a problem hiding this comment.
k is an encoded nat, which is never larger than 9 bytes - see rts/nat.h. There is a limit on the value size but it's very large and LMDB will throw an error if we exceed it.
|
@iamirzhan do you want me to combine this PR with #679 ? Note that AFAIK there's no way to have some source files that live only in the github repo, so you'll have to import all the files but just not use them in your internal build system. |
|
Response to the upstream request: That's totally reasonable. Until 1.0 is released we'll need to use our own local mods though. |
|
@simonhollis mentioned that we have 2k key size version of lmdb internally. So, I think the best way to go would be to put #679 here, but only for github repo? We will not include them to the internal build and also will not have these files in the internal repo EDIT: didn't notice your comment that it's not possible :) yeah, I suppose we will just not include them in the build |
Yes, we have an internal version of lmdb that enables -DMDB_MAXKEYSIZE=0, so we don't need the key length patch for internal builds. @simonmar could you then update this PR to add the Cabel package for OSS lmdb C-libs, and we'll build against our internal version. |
simonmar
force-pushed
the
lmdb-3
branch
2 times, most recently
from
26505c2 to
b354fbd
Compare
|
@simonhollis just to double-check, does your internal version also include the patch I mentioned above? Just |
|
@iamirzhan @simonhollis ready for import, and I rebased the rest of the stack |
Just dug into this. I think it's worth re-checking some calculations. What I'm seeing for our internal build doesn't have the patch but that without and |
|
@simonhollis you're right - sorry for the confusion. The patch doesn't seem to be necessary to get the 2kB key size. I'll revert to the pristine upstream code. |
3 participants
This commit adds an LMDB-based storage backend.
Why?
.cfiles).How do you use it?
Adding
--lmdbto thegleanorglean-servercommand line tells it to use LMDB for any DBs created. If this option is given, Glean will still be able to work with existing RocksDB DBs, including restoring them from remote storage. (and vice-versa, if--lmdbis not given, then restoring LMDB DBs from remote storage is still supported).How does it perform?
Testing indexing of Stackage using the Haskell indexer.
Note that backup with LMDB just copies the DB and compresses it, most of the time is spent in compression, which can be tuned to be either faster / less compression or slower / more compression. Currently using zstd level 8 which seems a reasonable compromise.
Also note that backup produces a squashfs which can be mounted directly (instantaneous restore with decompression on the fly). See #669
C++ Indexing
I indexed all of LLVM + Clang. This is the time to write the data to the DB and do deriving (since otherwise the indexing time is dominated by the C++ indexer itself):
Backup:
Restore:
db_lmdb_restore_unpack = false, see [lmdb #7] Optionally mount restored DBs using squashfs #669)Benchmarking with
query-benchGlass
glass-democlient liston 1000 files in the Stackage DB.rocksdb: 39s
lmdb: 31s
Caveats
The on-disk size of the DBs is larger than RocksDB by about 2x, although the backed-up size is about the same. This is mainly because LMDB doesn't do compression itself, however backup includes compression and the backup can be mounted directly as a squashfs (although mounting isn't implemented yet - we need to think about what happens if the server is restarted). I ran the Glass benchmark with the DB mounted as a squashfs and it was 32s vs. 31s, so hardly any difference.
There's no way to control the cache size, LMDB just maps pages into memory and relies on the OS to manage memory usage. It remains to be seen how well this works for a production server with many large DBs.
Key storage
LMDB doesn't support large keys, so I've used a strategy in which only the first ~2KB of the key is indexed. Searching for a key using a prefix larger than this may involve a linear search, but I think this is reasonable: we should never be looking up a fact using a large key. We can document this as a performance gotcha, just like sorting of fields. Indeed, we could use this same strategy with RocksDB to avoid duplicating large keys in the DB, which could potentially save a lot of space.