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:

Async trait methods must use static box futures. Not a dealbreaker, but requires extra cloning and boxing.
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.

Checklist

I have written the necessary rustdoc comments
[N/A] I have added the necessary unit tests and integration tests
I have passed cargo x (or cargo x --fast instead if the old tests are not modified) in my local environment.

Activity

Enable precise storage by passing Engine as a type parameter

009f65aApr 13

codecov[bot]6mo ago

Codecov Report

:x: Patch coverage is 53.62319% with 64 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
foyer-storage/src/engine/block/engine.rs	49.27%	35 Missing :warning:
foyer-storage/src/engine/noop.rs	0.00%	19 Missing :warning:
foyer/src/hybrid/cache.rs	50.00%	4 Missing :warning:
foyer-storage/src/store.rs	89.65%	3 Missing :warning:
foyer/src/hybrid/writer.rs	0.00%	3 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (481c3de) and HEAD (009f65a). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (481c3de) HEAD (009f65a)
3 1

Flag	BASE (481c3de)	HEAD (009f65a)
	3	1

Files with missing lines	Coverage Δ
foyer-bench/src/main.rs	`69.72% <100.00%> (-0.07%)`	:arrow_down:
foyer-storage/src/engine/mod.rs	`0.00% <ø> (-53.13%)`	:arrow_down:
foyer-storage/src/keeper.rs	`78.94% <ø> (-7.02%)`	:arrow_down:
foyer/src/hybrid/builder.rs	`79.21% <100.00%> (-16.72%)`	:arrow_down:
foyer-storage/src/store.rs	`72.61% <89.65%> (-13.82%)`	:arrow_down:
foyer/src/hybrid/writer.rs	`0.00% <0.00%> (-98.51%)`	:arrow_down:
foyer/src/hybrid/cache.rs	`47.72% <50.00%> (-37.03%)`	:arrow_down:
foyer-storage/src/engine/noop.rs	`6.12% <0.00%> (-30.84%)`	:arrow_down:
foyer-storage/src/engine/block/engine.rs	`67.14% <49.27%> (-22.68%)`	:arrow_down:

... and 45 files with indirect coverage changes

:rocket: New features to boost your workflow:

:snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
:package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

MrCroxx6mo ago

Hi, Renar. Thank you for your attention to and contribution to Foyer.

Frankly speaking, changing Engine from dyn dispatch to static dispatch is a very significant modification. It involves some changes that may contradict Foyer’s original design intent:

The design of Engine should remain flexible. Although Foyer currently has only one BlockEngine, Foyer still wants to keep the ability to support more types of engines (e.g. the previously existing but temporarily removed Set-Associated engine and Object engine) to handle workloads with entries of different sizes, as well as the capability for users to customize their own Engine. Although switching to static dispatch can still achieve this through more complex composition (in fact, earlier versions of Foyer did work this way), in practice we found that this approach is somewhat too complicated for users.
Foyer assumes that disk cache engines will often involve I/O, so dyn dispatch will not introduce a significant performance overhead.

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 Eq or other capabilities via type erasure.

Thank you again for your contribution, and you’re very welcome to discuss your ideas at any time in this thread.

sfc-gh-rnarubin6mo ago

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 load to pass &K into the storage engine, or a cloned K is fine too. I don't particularly need equivalent Q types, but the top-level HybridCache::get passes &Q, so there wasn't a good way around this. There might be an easier answer for dyn if it's constrained to the known type K, but that's difficult too.