fix(eviction): Tune eviction threshold in cache mode #4142
Conversation
BagritsevichStepan
changed the title
BagritsevichStepan
force-pushed
the
memory/tune-eviction-threshold-in-cache-mode
branch
from
cdd7fc0
to
b5e1611
Compare
BagritsevichStepan
changed the title
BagritsevichStepan
force-pushed
the
memory/tune-eviction-threshold-in-cache-mode
branch
5 times, most recently
from
5e6c873
to
2d7dd27
Compare
|
@adiholden I believe I've identified the issue: I'm calculating rss_threshold for each shard individually, but the current rss_memory value is a global metric for all shards |
BagritsevichStepan
force-pushed
the
memory/tune-eviction-threshold-in-cache-mode
branch
3 times, most recently
from
9bd58cc
to
fe22e03
Compare
BagritsevichStepan
force-pushed
the
memory/tune-eviction-threshold-in-cache-mode
branch
3 times, most recently
from
692a3fa
to
051412e
Compare
fixes dragonflydb#4139 Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
BagritsevichStepan
force-pushed
the
memory/tune-eviction-threshold-in-cache-mode
branch
from
651e8b0
to
2cc3610
Compare
|
Upd.:
|
|
|
||
| assert ( | ||
| memory_info["used_memory_rss"] > rss_before_connections | ||
| ), "RSS memory should have increased." |
There was a problem hiding this comment.
This is a check to do after adding the connections I believe.
In this stage of the test I think you should check that rss usage went below the threshold (rss_max_memory * 0.9)
| const double rss_oom_deny_ratio = ServerState::tlocal()->rss_oom_deny_ratio; | ||
|
|
||
| const bool should_evict = | ||
| eviction_memory_budget_threshold > 0.0 && GetFlag(FLAGS_enable_heartbeat_eviction); |
There was a problem hiding this comment.
why do you check eviction_memory_budget_threshold > 0.0 ?
nit: maybe rename this var should_evict -> eviction_enabled
|
|
||
| if (should_evict_depending_on_rss_memory) { | ||
| // Calculate how much rss memory is used by all shards | ||
| io::Result<io::StatusData> sdata_res = io::ReadStatusInfo(); |
There was a problem hiding this comment.
this RetireExpiredAndEvict function runs in the heart beat flow that runs every 10ms
today UpdateMemoryGlobalStats runs every 100ms, and I believe there is a reason for this, not to read the rss data so many time. In this logic now you run this read several times in this heartbeat flow (as the count of dbs) and I dont think we want this.
Further more you use above the used_mem_current that is updated in the UpdateMemoryGlobalStats and will not get updated in this for loop
There was a problem hiding this comment.
But used_mem_current can be updated by other threads
There was a problem hiding this comment.
I understand. The flow is indead very confusing!
I suggest to make this simpler by calculating the bytes goal to evict before the forloop on the dbs and once you enter the for loop you have the goal and you update it with the returned evicted byts value from FreeMemWithEvictionStep
In a later stage (not this PR) we can improve to update the rss fetch in this flow so it will be updated in case of eviction step if needed
| // If rss_oom_deny_ratio is set, we should evict depending on rss memory too | ||
| const bool should_evict_depending_on_rss_memory = should_evict && rss_oom_deny_ratio > 0.0; | ||
|
|
||
| size_t shard_memory_budget_threshold; |
There was a problem hiding this comment.
In addition I believe that it will be more readable if you write a function that will return the byte goal for eviction in this heartbeat invocation. It will have inside all this new logic that you introduced and call this function only if eviction is enabled
fixes #4139