[ENH] Disk and memory-backed cache with Foyer 0.10. #2890

rescrv · 2024-10-01T21:38:14Z

Description of changes

The lfu/lru/weighted_lfu caches didn't initialize a hybrid cache. This PR rewrites the chroma_cache crate to hide implementation details of the cache as much as possible. Noteworthy callouts from this PR:

We don't currently support cleaning up of tempfiles evicted from cache. This is status-quo.
cops-disk-cache-config-writer and cops-memory-cache-config-writer provide command-line arguments to generate the YAML for the Foyer cache.
Cache<K, V> is a trait. PersistentCache<K, V> allows for requiring persistence. All PersistentCache<K, V> can be cast to Cache<K, V>.

Test plan

How are these changes tested?

[] Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

github-actions · 2024-10-01T21:38:26Z

The lfu/lru/weighted_lfu caches didn't initialize a hybrid cache. This PR rewrites the chroma_cache crate to hide implementation details of the cache as much as possible. Noteworthy callouts from this PR: - We don't currently support cleaning up of tempfiles evicted from cache. This is status-quo. - cops-disk-cache-config-writer and cops-memory-cache-config-writer provide command-line arguments to generate the YAML for the Foyer cache. - Cache<K, V> is a trait. PersistentCache<K, V> allows for requiring persistence. All PersistentCache<K, V> can be cast to Cache<K, V>.

HammadB · 2024-10-03T20:38:17Z

rust/blockstore/src/arrow/block/types.rs

+    fn try_from(b: &Block) -> Result<Self, Self::Error> {
+        Ok(CachedBlock {
+            id: b.id,
+            data: b.to_bytes()?,


I think from_bytes/to_bytes will result in copies of the data, do we care?

We do. I'm debating on a two-level cache for persistent and non-persistent cache to avoid this problem, but I don't know how we'll do that.

HammadB · 2024-10-03T20:45:49Z

rust/blockstore/src/arrow/provider.rs

@@ -34,8 +34,8 @@ impl ArrowBlockfileProvider {
    pub fn new(
        storage: Storage,
        max_block_size_bytes: usize,
-        block_cache: Cache<Uuid, Block>,
-        sparse_index_cache: Cache<Uuid, SparseIndex>,
+        block_cache: Box<dyn Cache<Uuid, CachedBlock>>,


Does this mean that all cache accesses will use to_bytes/from_bytes? That seems not super great since its in the hot path?

HammadB · 2024-10-03T20:48:08Z

rust/blockstore/src/arrow/provider.rs

    }

    pub(super) async fn get(&self, id: &Uuid) -> Result<Option<Block>, GetError> {
-        let block = self.block_cache.get(id);
+        let block = self.block_cache.get(id).await.ok().flatten();


This means every access to the cache results in a copying of the data in the block, which means basically all queries copy potentially 100's of MB of data around.

+1, I agree that it seems like every cache access converts bytes to blocks which copies data (a block is 8MB so 8MB per access) and vice versa (unless I am missing something which is also likely)

HammadB · 2024-10-03T20:48:53Z

rust/blockstore/src/arrow/provider.rs

@@ -196,8 +197,9 @@ impl BlockManager {
    pub(super) fn new(
        storage: Storage,
        max_block_size_bytes: usize,
-        block_cache: Cache<Uuid, Block>,
+        block_cache: Box<dyn Cache<Uuid, CachedBlock>>,


Q: Why take a Box and turn it into an Arc instead of just taking an Arc?

The interface for creating creates a box. Consistency?

My bad for not noticing

rust/blockstore/src/config.rs

HammadB · 2024-10-03T20:52:49Z

rust/cache/src/bin/cops-disk-cache-config-writer.rs

+
+use chroma_cache::{CacheConfig, FoyerCacheConfig};
+
+fn main() {


nit: comment that this takes the default config and generates the yaml would be helpful

HammadB · 2024-10-03T20:53:25Z

rust/cache/src/bin/cops-memory-cache-config-writer.rs

+
+use chroma_cache::{CacheConfig, FoyerCacheConfig};
+
+fn main() {


nit / thought: might be nicer to have one binary that takes a flag for which config it generates

I chose to make it this way as it's clear from the tool which output you'll get. This allows us to have one tool per enum variant with exactly the options said variant needs. I'm not wedded to this idea, though.

good with me

HammadB · 2024-10-03T20:59:38Z

rust/cache/src/nop.rs

+use super::{CacheError, Weighted};
+
+/// A zero-configuration cache that doesn't evict.
+pub struct NopCache;


I think this is unused? If so, should we remove it?

It's wired up. I think being able to stub out the cache is useful, so I'd prefer to leave it. You have to set cache config to nop to use it, so the risk of inadvertently using it is low.

good with me

HammadB

Overall this looks good to me, my primary concern is to do with to_bytes/from_bytes on the CachedBlock impl. @sanketkedia wdyt?

sanketkedia · 2024-10-04T01:52:49Z

rust/blockstore/src/arrow/provider.rs

-    pub(super) fn cached(&self, id: &Uuid) -> bool {
-        self.block_cache.get(id).is_some()
+    pub(super) async fn cached(&self, id: &Uuid) -> bool {
+        self.block_cache.get(id).await.ok().is_some()


will this panic in case of errors? Should we instead catch the error and surface it?

.ok().is_some() should be written .is_ok(). It will not panic.

HammadB · 2024-10-04T21:17:01Z

rust/blockstore/src/arrow/block/types.rs

+/// Deserialize.
+#[derive(Clone, Debug)]
+#[repr(transparent)]
+pub struct RecordBatchWrapper(pub RecordBatch);


HammadB

Some comments on nits / potentially unused code that would be good to look at. Seems great otherwise! Thank you for chasing the solution

rescrv added 2 commits October 1, 2024 14:45

self-review cleanup

a1b7cf3

rescrv force-pushed the rescrv/foyer-0.10 branch from dae0120 to a1b7cf3 Compare October 1, 2024 21:45

rescrv added 4 commits October 1, 2024 16:40

be helpful on cache configuration errors

bb66221

add NOP cache and a nop enum that aliases lru, lfu, and weighted_lfu

ef8ec62

memory for hnsw cache config

6c89b15

build the memory cache in from_config

34e6f94

rescrv requested a review from HammadB October 2, 2024 16:29

backwards compatibility with the old config

19c3207

HammadB requested a review from sanketkedia October 3, 2024 00:35

HammadB reviewed Oct 3, 2024

View reviewed changes

rust/blockstore/src/config.rs Show resolved Hide resolved

HammadB reviewed Oct 3, 2024

View reviewed changes

sanketkedia reviewed Oct 4, 2024

View reviewed changes

rescrv added 2 commits October 4, 2024 11:34

ensure that Block can work with cache too, to eliminate to/from bytes

4783d70

Incorporate Hammad's suggestion to put ser/de on the block

26c06dd

HammadB reviewed Oct 4, 2024

View reviewed changes

HammadB approved these changes Oct 4, 2024

View reviewed changes

rescrv added 2 commits October 4, 2024 14:30

comments

8554615

cargo clippy

e060251

rescrv merged commit 8e9dbc9 into main Oct 4, 2024
70 checks passed

rescrv deleted the rescrv/foyer-0.10 branch October 4, 2024 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Disk and memory-backed cache with Foyer 0.10. #2890

[ENH] Disk and memory-backed cache with Foyer 0.10. #2890

rescrv commented Oct 1, 2024

github-actions bot commented Oct 1, 2024

HammadB Oct 3, 2024

rescrv Oct 4, 2024

HammadB Oct 3, 2024

HammadB Oct 3, 2024

sanketkedia Oct 4, 2024

HammadB Oct 3, 2024

rescrv Oct 4, 2024

HammadB Oct 4, 2024

HammadB Oct 4, 2024

HammadB Oct 3, 2024

HammadB Oct 3, 2024

rescrv Oct 4, 2024

HammadB Oct 4, 2024

HammadB Oct 3, 2024

rescrv Oct 4, 2024

HammadB Oct 4, 2024

HammadB left a comment

sanketkedia Oct 4, 2024

rescrv Oct 4, 2024

HammadB Oct 4, 2024

HammadB left a comment


		use chroma_cache::{CacheConfig, FoyerCacheConfig};

		fn main() {

[ENH] Disk and memory-backed cache with Foyer 0.10. #2890

[ENH] Disk and memory-backed cache with Foyer 0.10. #2890

Conversation

rescrv commented Oct 1, 2024

Description of changes

Test plan

Documentation Changes

github-actions bot commented Oct 1, 2024

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HammadB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HammadB left a comment

Choose a reason for hiding this comment