Fix the wrong usage for LRUCache #2267
Conversation
dangleptr
requested review from
darionyaphet,
critical27,
liuyu85cn,
bright-starry-sky and
panda-sheep
| if (FLAGS_enable_vertex_cache && vertexCache_ != nullptr) { | ||
| cacheData.emplace_back(v.get_id(), tag.get_tag_id(), tag.get_props()); | ||
| } | ||
| data.emplace_back(std::move(key), std::move(tag.get_props())); |
There was a problem hiding this comment.
One question,Does it cause duplicate indexes to be inserted? for example :
insert vertex ({vid1_tag1_version, prop1}, {vid1_tag1_version, prop2}...)
There was a problem hiding this comment.
When you insert {vid1_tag1_version, prop2}, the index generated by {vid1_tag1_version, prop1} will be deleted as our logic.
There was a problem hiding this comment.
Good one. I miss the logic. Maybe we could combine the inserts before calling kvstore::asyncPut
| list_.push_front(key); | ||
| map_.emplace(std::forward<key_type>(key), | ||
| std::make_tuple(std::forward<value_type>(value), list_.begin())); | ||
| } else { | ||
| // Overwrite the value | ||
| std::get<0>(it->second) = std::move(value); |
There was a problem hiding this comment.
also need to move this value to front of list ?
There was a problem hiding this comment.
If so, the operation becomes evict the old key then insert the new key.
I am not sure which way is better.
There was a problem hiding this comment.
Perhaps move to first make more sense since it is LRU, we could first check whether key exists, do what we want, then evict if necessary. We could use list::splice method instead of evict the old key then insert the new key.
There was a problem hiding this comment.
Try this list_.splice(list_.begin(), list_, std::get<1>(it->second));
src/common/base/ConcurrentLRUCache.h
Outdated
| VLOG(3) << "Insert key " << key << ", val " << value; | ||
| // insert the new item | ||
| // insert item into the cache, but first check if it is full | ||
| if (size() >= capacity_) { |
There was a problem hiding this comment.
what if I have a 10 capacity LRU,
which I have key 1-10 in it. (list 1 to 10)
Then if I insert a key 5,
it will pop 10, which is not really necessary
* Fix the wrong usage for LRUCache * Overwrite the old value when calling insert * Address comments * Address liuyu's comments * Address critical27's comments Co-authored-by: heng <heng.chen@vesoft.com>
* Fix the wrong usage for LRUCache * Overwrite the old value when calling insert * Address comments * Address liuyu's comments * Address critical27's comments Co-authored-by: heng <heng.chen@vesoft.com>
https://discuss.nebula-graph.com.cn/t/topic/948
As current usage, we can't ensure the data consistency between cache and data.
For example, reader data -> evict cache (write) -> write cache(in reader) will cause the dirty data forever before next write comes.
We should fix the problem with putIfAbsent when reading but insert overwrite when writing.
Of course, update/delete still has the problem, will fix them later.