-
Notifications
You must be signed in to change notification settings - Fork 198
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
Size aware cache #924
Size aware cache #924
Changes from 50 commits
02e9069
dc39c21
d99fd39
e39eaf9
876ec69
9500f4f
ffe18c6
79d9614
c1d83e6
ed4daca
3438b92
b5dc37c
b420cca
517b78e
7982aec
366fdf7
f2c10e4
0f95ce4
c2bb9c3
151305b
29fd940
3993af4
d094b80
d8691e1
8b9512b
fb7ec51
6325f0c
5f853e0
f22544c
7a26c27
d21e0bc
98a8469
5948e73
2f2209c
4204ea0
0892c9c
e6f4e8e
7f95615
e11bcf6
0948a5c
a23b905
3f06fff
43d8c91
57ea072
0903529
04480ff
1db3a74
9100eac
71a5aaa
f8327df
e925517
a9d3329
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package cache | ||
|
||
// WeightCalculator is a function that calculates the weight of a key-value pair in a Cache. | ||
// By default, the weight of a key-value pair is 1. Cache capacity is always specified in terms of | ||
// the weight of the key-value pairs it can hold, rather than the number of key-value pairs. | ||
// | ||
// Unless otherwise noted, Cache implementations are not required to be thread safe. | ||
type WeightCalculator[K comparable, V any] func(key K, value V) uint64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this generality or just give the cache a total memory limit will work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This generalization is needed to support the current use case. There isn't a trivial and cheap way we can determine the size of an arbitrary data structure, so we need the ability to compute the size for different data types. For things like blobs that are just an array of bytes, it's easy. For things like the chunks that are in a complex data structure, we need an abstraction like this. |
||
|
||
// Cache is an interface for a generic cache. | ||
type Cache[K comparable, V any] interface { | ||
// Get returns the value associated with the key, and a boolean indicating whether the key was found in the cache. | ||
Get(key K) (V, bool) | ||
|
||
// Put adds a key-value pair to the cache. After this operation, values may be dropped if the total weight | ||
// exceeds the configured maximum weight. Will ignore the new value if it exceeds the maximum weight | ||
// of the cache in and of itself. | ||
Put(key K, value V) | ||
|
||
// WithWeightCalculator sets the weight calculator for the cache. May only be called | ||
// when the cache is empty. The weight calculator should be an idempotent function that | ||
// always returns the same output given the same input. | ||
WithWeightCalculator(weightCalculator WeightCalculator[K, V]) error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be better to drop this method and pass it in from the constructor since it's required to have this calculator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If no weight calculator was provided, it would give every entry a weight of |
||
|
||
// Size returns the number of key-value pairs in the cache. | ||
Size() int | ||
|
||
// Weight returns the total weight of the key-value pairs in the cache. | ||
Weight() uint64 | ||
} |
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.
why? It's quite often the use cases will need thread safety.
Also please move this line down to the Cache
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.
With the cache accessor, we are forced to use a mutex in order to perform atomic operations on the cache and the map of in-progress operations. Since that is currently our only use case, adding thread safety to the cache would be redundant and would provide no immediate benefit.
I agree that having a thread safe cache may be useful if we want to reuse it as a general purpose cache. I see two options:
Cache
that adds thread safety to another cache implementation, e.g.func NewThreadSafeCache(cache Cache) Cache
. We could either write this now, or implement it later when we need it.Of these options, which would you prefer?
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 thought this intends to be a generic cache (per comment of
Cache
). If this has intended use case for cache accessor, then it's fine to keep it as it is.For the two options, they both look fine. Mutex locking is very cheap so there is no concern even if some use cases don't need concurrency.
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.
Currently there are no intended use cases outside of the cache accessor. Let's wait to add locking until we have a use case that justifies it.