erni27 / imcache Goto Github PK

We found the following snippet pretty common in our internal codebase.

val, ok := c.Get("key")
if ok && val == "value1" {
    _ = c.Replace("key", "value2", imcache.WithDefaultExpiration())
}

imcache can expose CompareAndSwap method in both Cache and Sharded types to make the operation atomic.

Method signature:

CompareAndSwap(key K, expected, new V, compare func (V, V) bool, exp Expiration) (swapped, present bool)

Add GetAndRemove to Cache and Sharded, it should atomically get the value under a given key and remove it from the cache.

Add GetMultiple method to both Cache and Sharded types.

Something like:

GetMultiple(keys ...K) map[K]V

It should get all values for given keys and return a map with key-value pairs. For Cache it should be an atomic operation, for Sharded reuse new GetMultiple Cache method unless there is a better option.

By default, a Cache doesn't have a max entries limit set. It doesn't maintain redundant data structure to apply LRU eviction policy if max entries limit exceeded.

But if a user create a new Cache with WithMaxEntriesOption with value less or equal to 0, it maintains LRU queue although it wouldn't be used.

This should be fixed and a Cache should behave exactly the same when a user doesn't specify WithMaxEntriesOption at all or provides it with value less or equal to 0.

Currently there is only one hasher: DefaultStringHasher64, it's possible to generate hash for any type automatically, take a look: https://github.com/tidwall/hashmap/blob/e128c7c0599b6d15d68345d07dd373cf26c8904d/map.go#L45. This method uses the memory bytes directly to generate hash and Theine also use this.

By the way do you find any bug or think something can be improved about Theine? Because it's not popular, I can see all star history on my home page.

Because this is a library, users have to carry around all dependencies, so the fewer the better.

I did a cursory look - is go-cmp only used in tests?

Can you get away with regular comparisons and/or using reflect.DeepEqual or is there a real need for something different?

Most of the unit tests are simple, table-driven tests. They test both Cache and Sharded types. Setup for both types is the same though. Hence we can reduce boilerplate code by adding setup func in test structures taking imcache interface as an argument and do the setup there instead of do the same setup for both Cache and Sharded.

https://github.com/erni27/imcache/blob/master/imcache_test.go#L31 - one of the tests to be improved.

https://github.com/erni27/imcache/blob/master/imcache_test.go#L544 - test already improved.

imcache currently supports only least recently used (lru) eviction policy if MaxEntries limit is set. One can set the limit using WithMaxEntriesOption (https://github.com/erni27/imcache/blob/master/option.go#L63). Explore other eviction policies (especially lfu and shortest ttl) that can be used in imcache. WithMaxEntriesOption func should be deprecated and new func WithMaxEntriesLimitOption should be introduced to set both the cache max entries limit and the eviction policy.

func WithMaxEntriesLimitOption[K comparable, V any](n int, policy EvictionPolicy) Option[K, V]

Internal abstraction to manage eviction sequence may need to be updated (https://github.com/erni27/imcache/blob/master/eviction.go#L57).

Currently imcache supports read operations like Get, GetAll or GetMultiple. imcache supports active eviction if the entry is expired (the entry is removed from the cache whenever it is read) hence it acquires the mutual exclusion lock on the cache while reading. The mutual exclusion lock is also required if the max entries limit is enabled (imcache maintains additional data structure to maintain the eviction queue that could be updated on read operations - depending on the chosen eviction policy). Moreover, sliding expiration requires the expiration date update whenever the entry is read.

Add Peek, PeekAll and PeekMultiple methods mirroring the functionality of the Get methods but acquiring the shared access (RWMutex). They shouldn't actively evict the expired entries, slide the expiration and update the eviction queue (if enabled).

Currently if EvictionCallback is set imcache blocks during the entry eviction process until EvictionCallback is done. Cache (or Sharded) doesn't hold a lock though so concurrent requests are handled.
Example: https://github.com/erni27/imcache/blob/master/imcache.go#L156.

Current behaviour could be annoying if EvictionCallback is slow. Spawning a new goroutine and calling EvictionCallback inside it should be considered.

Introduce new struct Metrics containing simple metrics (hits, misses, insertions, deletions and evictions).

Extend an API by adding Metrics method returning Cache (or Sharded) metrics.

Currently imache does not use named result parameters.

Although it is not needed from the functionality point of view, it could increase the code readability.

Instead of:

Get(key K) (V, bool)

Use:

Get(key K) (val V, present bool)

One can immediately tell what the return parameters mean without reading the doc comments. Extremely useful for more ambiguous methods.

Add a new README section with brief comparison between imcache and other in-memory cache libraries.

It would help users looking for an in-memory cache library to better understand if imcache is a great fit for them.

To test the eviction callback functionality the following mock struct is used https://github.com/erni27/imcache/blob/master/imcache_test.go#L1018.

Since the eviction callbacks are invoked in the separate goroutine(s), the assertion methods should wait reasonable amount of time to make sure that the callback has eventually been called / has not been called (e.g. https://github.com/erni27/imcache/blob/master/imcache_test.go#L1029).

Most of the assertion methods though are flaky, e.g. HasNotBeenCalledWith (https://github.com/erni27/imcache/blob/master/imcache_test.go#L1050) doesn't ensure the callback really has not been called.

Revisit evictionCallbackMock and improve the flaky assertions. Make sure all tests use the proper assertion methods.

Although imcache supports entry ttl, it doesn't support policy evicting the entries by their expiration (ttl) timestamp (currently supported policies: lru, lfu and rand).

Add the new eviction policy EvictionPolicyTTL. The binary heap could be used as the backing data structure.