Report to Log the Index size per CF - LRU Cache Only (#338) #368
Conversation
| const Slice& key, void* value, const CacheItemHelper* helper, | ||
| size_t charge, Handle** handle = nullptr, | ||
| Priority priority = Priority::LOW, | ||
| Cache::ItemOwnerId item_owner_id = kUnknownItemId) override; |
There was a problem hiding this comment.
Is default value needed here?
There was a problem hiding this comment.
Is default value needed here?
This function overrides the one in Cache, which does have a default value. If I want to allow the user to call Insert() without explicitly specifying the item_owner_id, it must have a default (and It should be the same default). The fact that the base class has a default, doesn't make this inherit that behaviour.
There was a problem hiding this comment.
As for your comment about many small changes. I agree. However, since I am changing the signature of the Insert function and the signature of the callback function on which the solution is based, I see no other option.
If, in addition, I will not use a default value for the item_owner_id, there will be even more changes in calls that currently may stay as is because that default saves us the need to change them as well.
cache/cache.cc
Outdated
| std::lock_guard<std::mutex> lock(free_ids_mutex_); | ||
| // There must be at least 1 list element on the free_ids_ list since | ||
| // we num_free_ids_ must have been >= 1 on entry if we got here. | ||
| // Asserting to catch the bug during testing, however, this is a |
There was a problem hiding this comment.
All that code is to avoid locking mutex when checking if free list is non-empty upfront? But isn't allocation itself very rare? And what happens when another thread changes these values at the same time?
There was a problem hiding this comment.
Yes, the code if for that. The code is supposed to be thread safe (under the assumptions described in the comments). Please let me know if you do find a bug in it.
However, if you feel I should just use the mutex at the start since this is a rare event, that will simplify the code and the code may be removed.
cache/lru_cache.cc
Outdated
| @@ -468,7 +469,7 @@ void LRUCacheShard::Promote(LRUHandle* e) { | |||
| e->IsHighPri() ? Cache::Priority::HIGH : Cache::Priority::LOW; | |||
| s = Insert(e->key(), e->hash, /*value=*/nullptr, 0, | |||
| /*deleter=*/nullptr, /*helper=*/nullptr, /*handle=*/nullptr, | |||
| priority); | |||
| priority, Cache::kUnknownItemId); | |||
There was a problem hiding this comment.
Why is it unknown here?
There was a problem hiding this comment.
Agree. I have changed this to pass the item owner id of the promoted handle (assuming that was what bothered you)
udi-speedb
force-pushed
the
19-index-size-per-cf
branch
from
b09b016
to
e71353d
Compare
|
@erez-speedb, @Yuval-Ariel I have just noticed that I haven't squashed the commits. |
udi-speedb
force-pushed
the
19-index-size-per-cf
branch
from
e71353d
to
3730ddf
Compare
udi-speedb
force-pushed
the
19-index-size-per-cf
branch
from
0026c25
to
3730ddf
Compare
|
@speedbmike - I apologize. I did a test on this PR together with @maxb-io. @maxb-io changed the policy to invalidate a pr approval once new commits are pushed to the pr. So I pushed a dummy commit and now I have forced pushed the branch again without the dummy commit. |
udi-speedb
force-pushed
the
19-index-size-per-cf
branch
from
3730ddf
to
d90b1a9
Compare
No description provided.