Enable precise storage by passing Engine as a type parameter#1287
Enable precise storage by passing Engine as a type parameter#1287sfc-gh-rnarubin wants to merge 1 commit intofoyer-rs:mainfrom
Conversation
Previously, the storage Engine trait was referenced as a Arc<dyn Engine> in the inner cache Store. While this enabled runtime flexibility to change the engine dynamically, it required that all methods be dyn compatible. This had several consequences: 1. Async trait methods must use static box futures. Not a dealbreaker, but requires extra cloning and boxing. 2. Trait methods cannot introduce generic parameters. This forbids eq-based key comparison: `fn load<Q: Hash + Equivalent<K>>(&self, key: &Q) -> ...` As a result, the existing Engine definition only passed the hash and not a full key to load objects. In order to support more expressive storage engines (precise key lookup, key sorting, etc), this change updates the Engine trait to accept `key: &Q, hash: u64` for most methods (hash is preserved to save recomputation). The key equality check is also moved into the engine itself, rather than the Store type as before. This unfortunately makes the trait no-longer dyn compatible. To resolve that problem, the Engine trait is now passed through foyer as a generic type parameter. It can no longer be set dynamically, a concrete engine config must be provided at the builder definition. This seems like a reasonable compromise: one is likely to choose the storage engine based on their hardware requirements and not runtime configuration. A nice side effect is that the async trait methods can now also use concrete and non-static futures, which enables fewer clones and boxes. This change also increases pub visibility on PieceRef to make external Engine implementations possible.
Codecov Report❌ Patch coverage is
... and 45 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hi, Renar. Thank you for your attention to and contribution to Foyer. Frankly speaking, changing
However, it must be admitted that passing the key as a parameter to the engine is indeed important for engines that rely on ordering. Let me see if it’s possible to expose Thank you again for your contribution, and you’re very welcome to discuss your ideas at any time in this thread. |
|
Yes it's a pretty big interface change, so I understand if you don't want to adopt this. I'd be curious to see if there's a way to do it while using dyn. My requirements are actually narrower than implemented here: I need |
What's changed and what's your intention?
Previously, the storage Engine trait was referenced as a
Arc<dyn Engine>in the inner cache Store. While this enabled runtime flexibility to change the engine dynamically, it required that all methods be dyn compatible. This had several consequences:fn load<Q: Hash + Equivalent<K>>(&self, key: &Q) -> ...As a result, the existing Engine definition only passed the hash and not a full key to load objects.In order to support more expressive storage engines (precise key lookup, key sorting, etc), this change updates the Engine trait to accept
key: &Q, hash: u64for most methods (hash is preserved to save recomputation). The key equality check is also moved into the engine itself, rather than the Store type as before. This unfortunately makes the trait no-longer dyn compatible.To resolve that problem, the Engine trait is now passed through foyer as a generic type parameter. It can no longer be set dynamically, a concrete engine config must be provided at the builder definition. This seems like a reasonable compromise: one is likely to choose the storage engine based on their hardware requirements and not runtime configuration.
A nice side effect is that the async trait methods can now also use concrete and non-static futures, which enables fewer clones and boxes.
This change also increases pub visibility on PieceRef to make external Engine implementations possible.
Checklist
cargo x(orcargo x --fastinstead if the old tests are not modified) in my local environment.