Add expirable LRU implementation #68
Conversation
|
@jefferai would you be so kind as to look into it and\or include other reviewers who might review it as well? |
|
Gentle ping for @jefferai |
|
Another gentle ping for @jefferai. |
|
@paskal do be honest I don't think I'm the right person to review this. I don't have a need for it so I also don't know what is needed in such an implementation that we would feel comfortable committing to in terms of API. |
|
I'll close this one as I don't want to maintain the fork on my own and I do understand your concern regarding committing for new API support. I hope if someone will need exactly this thing, he or she will retrieve this code and will be able to use it. |
| } | ||
|
|
||
| // noEvictionTTL - very long ttl to prevent eviction | ||
| const noEvictionTTL = time.Hour * 24 * 365 * 10 |
There was a problem hiding this comment.
If a TTL of 0 is defined as turning TTL off, why do you need this at all? Couldn't the check below simply be if res.ttl != 0 && ....
There was a problem hiding this comment.
Current expiration logic is very straightforward: ent.Value.(*expirableEntry).expiresAt = now.Add(c.ttl)
Considering zero a special case after the cache creation would complicate the code I believe. I would rather have this extra complexity on cache instance setup rather than inside it's logic. I haven't thought of any more elegant alternative, but it definetly doesn't mean it's not possible to find it.
There was a problem hiding this comment.
@jefferai would you be so kind as to look into discussions in this PR?
There was a problem hiding this comment.
@jefferai gentle ping
|
|
||
| sync.Mutex | ||
| items map[interface{}]*list.Element | ||
| evictList *list.List |
There was a problem hiding this comment.
Rather than using a list where you have to keep scanning through, I think you'd be better off using a priority queue. See https://github.com/hashicorp/vault/tree/master/sdk/queue and the Golang doc it links to. That way you simply add items to the queue where the sorting is by expiration time and only have to keep track of the first item in the queue in terms of expiration. This would also let you get rid of Close; a simple one-off timer would be the only goroutine you need to keep checking the first element.
There was a problem hiding this comment.
@jefferai, thanks for your thoughts! I understand the benefits of using a heap, but I don't understand how it would change anything.
removeElement is constant complexity now, it stays the same.
DeleteExpired complexity stays the same, as to delete all expired elements we either need to traverse whole dataset (O(N)) or go trough sorted list until we find first non-expired element (O(N*logN)) or check top element of the heap until we find a non-expiring element, which I believe would be exact same complexity as sorted list, as single heap rebalance would be O(logN) multiplied by N elements we want to delete (e.g. whole dataset during lifetime of the program).
If we want to have deleteExpired call in a goroutine, we need ability to call Close() to kill it. If I understand you correctly and you want me to not to call deleteExpired by timer, I don't understand which alternative you propose: even if we set one-time timer on new entry which will wait for it to expire and then clean it up, or clean up everything expired so far, we still want the ability to kill it explicitly.
Would you be so kind as to elaborate on what you want to see changed in that code behavior and what's the benefit of using heap for it?
There was a problem hiding this comment.
I think it's better to split this discussion into two separate threads: is there any way to get rid of the Close method (which is only possible if we won't have any goroutines spawned as I think) and is there a way to make this code faster.
|
@paskal Left some comments. I think it could benefit from a bit of restructuring that would clean up the API and be more efficient and clear. |
|
@jefferai could you please look into it once more? |
|
I don't think it will be merged. I've created a fork with expirable cache added, released as v0.6.0: |
|
Sure, I'd love to have this in golang-lru, |
|
Created #113 based on the current master. |
My attempt at expirable cache implementation, similar to what was done in #41. I've tried to make new cache as close as possible to the existing one in terms of code style and behavior.
The only difference which I did leave in place is that
size=0on this type of cache means unlimited, which makes sense with the expirable cache.Original work was done in lcw but it turned out that I don't need it there and I thought it would be nice to add my changes to upstream (this repo).