-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lmdb store #10
Add lmdb store #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I was thinking we need to change the api to iterator style rather than in memory collections but one thing at a time.
Yeah Ashanti and I talked about that and I had a bit of a crack but not as easy as I first thought. Agreed that should be next though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, fundamentally. I made some minor comments about things to change, like the From<&[u8]>, and the DRYness, which would be nice to fix before. Also @AshantiMutinta didn't request changes, but she made some important points that should probably be addressed before merging.
// Thes flags make writes waaaaay faster by async writing to disk rather than blocking | ||
// There is some loss of data integrity guarantees that comes with this | ||
.set_flags(EnvironmentFlags::WRITE_MAP | EnvironmentFlags::MAP_ASYNC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be OK though right? Since our stores are all monotonic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it opens up the possibility of corrupting the whole database by writing to the wrong places in memory since it doesn't verify before syncing to disk. But Rust should stop us from doing that right?
Co-Authored-By: Michael Dougherty <michael.dougherty@holo.host>
Co-Authored-By: Michael Dougherty <michael.dougherty@holo.host>
keen to see if this translates to something like indexed db in the future in the browser :) |
Yeah I saw your post in the mattermost. That could probably even better support EAV queries as I believe it has support for custom indexes as well as sorted keys. |
PR summary
This PR adds a new implementation of both CAS and EAV that uses LMDB. This is the main store used by the Monero cryptocurrency as well as being used my Mozilla in the firefox browser. As such it is well supported and has excellent Rust bindings currently maintained by Mozilla.
LMDB maintains a sorted order of keys which, with a clever key naming scheme, allows for a huge optimization in performing queries that match exactly on an entity (the most common type of query). Rather than iterating the entire database it is possible to jump to the beginning of EAVI entries for the entry and read until the next entry.
Benchmarks show comparable performance to PickleDB for CAS and EAV get_all retrievals but a significant improvement in get_exact queries.
Future questions
Review checklist