[core] Enable heterogeneous access for LRU cache #48954
Conversation
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
dentiny
force-pushed
the
hjiang/lru-cache-optimization
branch
from
0b103a2
to
05b67f7
Compare
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
| @@ -90,7 +96,8 @@ class SharedLruCache final { | |||
| // Delete the entry with key `key`. Return true if the entry was found for | |||
| // `key`, false if the entry was not found. In both cases, there is no entry | |||
| // with key `key` existed after the call. | |||
| bool Delete(const Key &key) { | |||
| template <typename KeyLike> | |||
| bool Delete(KeyLike &&key) { | |||
There was a problem hiding this comment.
why do we need a move in Delete? It's possible user wants to delete then still use its key.
There was a problem hiding this comment.
Also even if you want move semantics you want to pass in by value?
There was a problem hiding this comment.
It's perfect forward with universal reference not move.
There was a problem hiding this comment.
can forward instead of pass by value in headers, slightly more efficient, but only makes sense for Put where you can possibly forward rvalue in, underlying cache_ functions used for Get and Delete take const lvalue anyways so it doesn't matter...
There was a problem hiding this comment.
Put takes ownership of the key, so I take it by value;
Deletion and lookup only borrows reference, so perfect forwarding.
| @@ -34,22 +47,21 @@ TEST(SharedLruCache, PutAndGet) { | |||
|
|
|||
| // Check put and get. | |||
| cache.Put("1", std::make_shared<std::string>("1")); | |||
There was a problem hiding this comment.
write a comment about the heterogeneous key is tested here.
There was a problem hiding this comment.
No, I don't use heterogeneous access for put operation, since the ownership is transferred anyway.
| TestClass obj2{"hello"}; | ||
| SharedLruCache<TestClass, std::string> cache{2}; | ||
| cache.Put(obj1, std::make_shared<std::string>("val")); | ||
| auto val = cache.Get(obj2); |
There was a problem hiding this comment.
I guess we want to test cache.Get(absl::string_view{"hello"}) here?
There was a problem hiding this comment.
No, heterogeneous lookup is provided by flat hash map implementation.
To achieve that, we need to implement more features, like comparison between TestClass and std::string.
Reference: https://github.com/abseil/abseil-cpp/blob/c7cf999bda8390d2dd294ef903716a80135e6f4c/absl/container/flat_hash_map.h#L86-L94
There was a problem hiding this comment.
The intended use case for heterogeneous lookup here is C++ native string types, including std::string_view, std::string and const char*.
For example, for a flat_hash_map<string, string>, you don't want to create a temporary string for lookup right? A string view, if possible, should suffice.
All of them have been implemented by hash map. Unfortunately not for STL unordered map implementation.
There was a problem hiding this comment.
right, can we have a test case that heterogeneous lookup is used?
There was a problem hiding this comment.
I think I already did?
The above PutAndGet test case is using std::string for storage type, but std::string_view for delete and lookup type?
|
@rynewang / @dayshah |
The thread-safe implementation should perfect forward what's provided into the non-thread-safe version, whatever value category it belongs to. |
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
|
@rynewang I have updated the implementation based on our offline discussion. |
Signed-off-by: hjiang <[email protected]>
dentiny
changed the title
As titled, allow heterogeneous lookup and insertion.