Switch LRU to internal cache implementation #20
Conversation
| @@ -24,20 +24,20 @@ type LoadingCache struct { | |||
| onEvicted func(key string, value interface{}) | |||
|
|
|||
| sync.Mutex | |||
| data map[string]*cacheItem | |||
| data map[string]*list.Element | |||
There was a problem hiding this comment.
I get the idea, but hate this implimentation. Keeping list element, wich is a part of linked list container, inside of map looks strange and confusing
There was a problem hiding this comment.
Other way around is list element inside CacheItem, I did it in #21.
| @@ -24,20 +24,20 @@ type LoadingCache struct { | |||
| onEvicted func(key string, value interface{}) | |||
|
|
|||
| sync.Mutex | |||
| data map[string]*cacheItem | |||
| data map[string]*list.Element | |||
| evictList *list.List | |||
There was a problem hiding this comment.
I'd suggest to change this evictList to list of keys. This way we will duplicate keys but the evictList can be a nice way to add sorting to the primary map, For lru it will push to top on access, for TTL it will be sorted natively by insertion order.
There was a problem hiding this comment.
We have "natural" sorting for both LRU and LRC you mentioned already in place.
*list.Element is used because Go lacks "generics", I'll need to copy-paste list implementation for our type to change it's values types. Do you think we should do that?
There was a problem hiding this comment.
no, my suggestion was to keep a sorted list for evicted keys only and leave items alone. I.e. data will be map of naked interface{} (as before) and evictList will hold keys only
| key string | ||
| ts time.Time | ||
| // removeOldest removes the oldest item from the cache. Has to be called with lock! | ||
| func (c *LoadingCache) removeOldest() { |
There was a problem hiding this comment.
this name is misleading. As far as I undesrstand it is oldes for TTL case, but not in LRU case
There was a problem hiding this comment.
Oldest by creation time in case of LRC, oldest by access time in case of LRU.
There was a problem hiding this comment.
well, "oldest" means an age, at least this is how I read it. In the case of LRU this is not "age" but rather "access time". Not sure what the right name here, but "oldest" is misleading for real
| } | ||
|
|
||
| // deleteExpired deletes expired records. Has to be called with lock! | ||
| func (c *LoadingCache) deleteExpired() { |
There was a problem hiding this comment.
not sure what expired exactly means for LRU cache. TTL (expiresAt) shoudn't play any role in this case
There was a problem hiding this comment.
It works only if Expired is set to something less than noEvictionTTL. The function does exactly what is written here: deletes expired entries.
It won't work for the LRU cache without the expiration.
There was a problem hiding this comment.
the point I'm making - LRU should not be tied to expiration in any way. Maybe it was the part of my cache implementation, but we want to simplify it and make LRU really limited by max size only and evict in LRU mode only based on max keys and the total size (indirectly). Supporting TTL in LRU mode makes all of this way too confusing
This version is basically hybrid of ours and hashicorp/golang-lru. It turned out we still support expirable LRU cache, we just don't use it in lcw yet.