cache: update LRU cache when putting same key but different value #424
Conversation
| if v == value { | ||
| return | ||
| } else { | ||
| lru.priority.Remove(lru.priority.Front()) |
There was a problem hiding this comment.
Hi, I'm not sure why this is necessary. Why remove the most used element?
There was a problem hiding this comment.
Hi, umm... When lru.get()#35 was called and found the element value, lru.priority.MoveToFront(element) is supposed to be called here. So, the element came to the front. And that's the reason why I remove the front value.
Is this wrong?
There was a problem hiding this comment.
Ok, I understand now. So it is not wrong, but you are using a side-effect of the get() function. If at some future time get() does not do that, this code would remove a different element.
Instead, find the element, and remove it. That will always work.
There was a problem hiding this comment.
Yes, I admit that it has a side effect as you mentioned. But I think golang's container/list doesn't have the "efficient" find function (using Next()/Prev() loop would be O(n), but I don't want to use them for the cache code here.). Do you have any idea to find the element to remove efficiently?
There was a problem hiding this comment.
Ah, find code with Next() loop would be O(n) generally, but in this case, the element is supposed to be moved in the front. Thus, even if I use the loop code, most probably the loop can be finished in the first loop.
I will update this with writing the Next() and loop code to find element.
|
You probably want to force a removal of this element, then call Put() again instead. |
|
Thank you for your advice. I updated. |
| if old := lru.find(lru.priority.Front(), v); old != nil { | ||
| lru.priority.Remove(old) | ||
| } else { | ||
| clog.Errorf("read cache is corrupted. Please restart the process.") |
| func (lru *cache) find(e *list.Element, v interface{}) *list.Element { | ||
| if e == nil { | ||
| return nil | ||
| } else if e.Value.(kv).value == v { |
There was a problem hiding this comment.
change else if to if. we already return at line 81.
| if reflect.DeepEqual(v, value) { | ||
| return | ||
| } else { | ||
| // Actually find() is not necessary, but it makes sure to remove |
There was a problem hiding this comment.
not sure if we want to be extra safe here.
There was a problem hiding this comment.
Umm... @lpabon how do you think?
|
lgtm. defer to @lpabon. Can you take another look? Thanks. |
cache: update LRU cache when putting same key but different value
When same key but different value was put on the cache, current code
doesn't replace the cache. This patch changes to find the same value
cache and replace it when the same key cache was put.