Skip to content

[common/cache] add a basic lru cache#1600

Merged
databend-bot merged 9 commits into
databendlabs:masterfrom
PsiACE:cache
Sep 1, 2021
Merged

[common/cache] add a basic lru cache#1600
databend-bot merged 9 commits into
databendlabs:masterfrom
PsiACE:cache

Conversation

@PsiACE

@PsiACE PsiACE commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/

Summary

  • a basic lru cache for memory/disk, derived from sccache/lru_disk_cache

TODO

  • Reconstructing the interface for memory cache, provide a common abstract interface
  • Reconstructing disk cache
  • Integrating with datafuse
  • Polishing, docs and more

Changelog

  • New Feature
  • Improvement

Related Issues

Fixes #1429

Test Plan

Unit Tests

Stateless Tests

PsiACE added 2 commits August 25, 2021 12:24
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
@databend-bot

Copy link
Copy Markdown
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@databend-bot databend-bot added pr-feature this PR introduces a new feature to the codebase pr-improvement labels Aug 25, 2021
@ZhiHanZ

ZhiHanZ commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

folly has some interesting LRU cache implementations:
https://github.com/facebook/folly/blob/master/folly/container/EvictingCacheMap.h

@ZhiHanZ

ZhiHanZ commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

one question about disk based LRU cache, could we assume the size for persistence storage, for example, in sccache, we could set the cache size to 10Gi or more

Signed-off-by: Chojan Shang <psiace@outlook.com>
@PsiACE

PsiACE commented Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

one question about disk based LRU cache, could we assume the size for persistence storage, for example, in sccache, we could set the cache size to 10Gi or more

At present there are no differences between it and the cache in sccache, except for some differences in dependencies and features.

@codecov-commenter

codecov-commenter commented Aug 25, 2021

Copy link
Copy Markdown

Codecov Report

Merging #1600 (22bc1a0) into master (7b2afe7) will increase coverage by 0%.
The diff coverage is 92%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1600    +/-   ##
=======================================
  Coverage      71%     72%            
=======================================
  Files         568     575     +7     
  Lines       33130   33731   +601     
=======================================
+ Hits        23823   24378   +555     
- Misses       9307    9353    +46     
Impacted Files Coverage Δ
...uery/src/datasources/remote/remote_meta_backend.rs 0% <ø> (ø)
common/cache/src/disk_cache.rs 71% <71%> (ø)
common/cache/src/cache/lru.rs 97% <97%> (ø)
common/cache/src/disk_cache_test.rs 98% <98%> (ø)
common/cache/src/cache/lru_test.rs 100% <100%> (ø)
common/cache/src/meter/count_meter.rs 100% <100%> (ø)
common/cache/src/meter/file_meter.rs 100% <100%> (ø)
common/cache/src/meter/heap_meter.rs 100% <100%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b2afe7...22bc1a0. Read the comment docs.

@bohutang

bohutang commented Aug 25, 2021

Copy link
Copy Markdown
Member

Tips:
It would better define the api(the api keeping TODO is ok) first, then let datafuse integrate it, and finally make it workable.
From your todo list, that's what you do 👍

PsiACE added 5 commits August 30, 2021 23:36
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
@PsiACE
PsiACE marked this pull request as ready for review September 1, 2021 03:52
@databend-bot

Copy link
Copy Markdown
Member

Hello @PsiACE, 🎉 Thank you for opening the pull request! 🎉
Your pull request state is not in Draft, please add Reviewers or Re-request review again!
FuseQuery: @BohuTANG @sundy-li @zhang2014
FuseStore: @drmingdrmer @dantengsky
Or visit datafuse roadmap for some clues.

Signed-off-by: Chojan Shang <psiace@outlook.com>
}

/// An LRU cache of files on disk.
pub type LruDiskCache = DiskCache<LruCache<OsString, u64, DefaultHashBuilder, FileSize>>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can easily define another disk cache based on a different policy by simply implementing the common_cache::cache::Cache trait.

Comment thread common/cache/src/cache.rs
use crate::Meter;

/// A trait for a cache.
pub trait Cache<K, V, S, M>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An almost universal Cache trait

@dantengsky dantengsky left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, 👍

@databend-bot

Copy link
Copy Markdown
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
databend-bot merged commit 8f95386 into databendlabs:master Sep 1, 2021
@PsiACE
PsiACE deleted the cache branch September 1, 2021 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-feature this PR introduces a new feature to the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[common] common/cache mod

6 participants