-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add maxmemory-reserved-scale parameter for evicting key earlier to avoid OOM #831
base: unstable
Are you sure you want to change the base?
Conversation
e31dd8b
to
b8a98df
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #831 +/- ##
============================================
+ Coverage 70.65% 70.72% +0.06%
============================================
Files 114 114
Lines 61799 63091 +1292
============================================
+ Hits 43664 44619 +955
- Misses 18135 18472 +337
|
b8a98df
to
ba13a0c
Compare
ba13a0c
to
e27e9de
Compare
this seem like a interesting feature, did not review, just drop a comment that approve the concept. |
I also like the idea. I just haven't really spent enough time thinking about it. Memory management is a big area in Valkey we need to improve. |
e27e9de
to
0a68013
Compare
0a68013
to
9596c78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a quick look.
src/evict.c
Outdated
} else if (mem_reported <= server.maxmemory && !level) | ||
return C_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. add {} to match the if
part?
} else if (mem_reported <= server.maxmemory && !level) | |
return C_OK; | |
} else if (mem_reported <= server.maxmemory && !level) { | |
return C_OK; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address.
src/server.c
Outdated
server.maxmemory_available = | ||
(unsigned long long)server.maxmemory / 100.0 * (100 - server.maxmemory_reserved_scale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract this formula to an inline function? this has been repeated 3 times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address. I just create an inline function in server.h
src/evict.c
Outdated
if (server.maxmemory_reserved_scale) { | ||
mem_tofree = mem_used - server.maxmemory_available; | ||
} else | ||
mem_tofree = mem_used - server.maxmemory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this check needed? if server.maxmemory_reserved_scale
is 0, server.maxmemory_available
would be equal to server.maxmemory
.
similarly, this would apply to the other changes in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address.
+1 on introducing "back pressure" earlier. I feel that this could be used with `maxmemory_eviction_tenacity" to give an even smoother eviction experience. |
35437eb
to
db966fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hwware! LGTM overall. Can you add some tests too?
src/server.h
Outdated
@@ -3010,6 +3012,9 @@ void trimStringObjectIfNeeded(robj *o, int trim_small_values); | |||
static inline int canUseSharedObject(void) { | |||
return server.maxmemory == 0 || !(server.maxmemory_policy & MAXMEMORY_FLAG_NO_SHARED_INTEGERS); | |||
} | |||
static inline void calculateMaxAvailableMemory(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are not returning any value from this function but updating a state in server
, I feel that `update would be a better verb to indicate that this is more of a "method" than a "function".
static inline void calculateMaxAvailableMemory(void) { | |
static inline void updateMaxAvailableMemory(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address
Sure, ready to work, Thanks |
f7cc8c8
to
7a5584b
Compare
@@ -398,11 +398,12 @@ int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *lev | |||
if (total) *total = mem_reported; | |||
|
|||
/* We may return ASAP if there is no need to compute the level. */ | |||
if (!server.maxmemory) { | |||
if (!server.maxmemory_available) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, I feel that it is more preferable to model this new setting as a "soft" maxmemory
, which can trigger key eviction earlier but won't cause OOM to be returned before hitting the actual maxmemory
. Otherwise, we effectively create an alias of maxmemory
. More specifically, I think performEviction
should only return EVICT_FAIL
when the memory usage goes beyond the real maxmemory
.
Additionally, since getMaxmemoryState
is also used outside of the OOM prevention path such as in VM_GetUsedMemoryRatio
, we should consider parameterizing maxmemory
and having it passed in by the caller instead, so that we can maintain the same semantics in these externally facing scenarios.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like what you suggested, if we can think when "maxmemory_available" is available, it is a soft maxmemory. Then you maybe think we should not return OOM, in the case, how we can return message to client, any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right - how about just processing the command normally?
- if
used_memory
is below soft max, no change to the existing logic - if
used_memory
is above soft max but below hard max, trigger key eviction and continue with the normal processing - if
used_memory
is above hard max, no change to the existing logic, i.e., trigger key eviction and fail the command if theused_memory
is still above hard max after the eviction)
200b203
to
4da32a2
Compare
4da32a2
to
510265f
Compare
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
510265f
to
c6db98b
Compare
Reference: #742 and https://learn.microsoft.com/en-us/azure/azure-cache-for-redis/cache-best-practices-memory-management (Azure)
Generally, when clients set maxmemory-policy as allkeys-lru or other memory eviction policies, and maxmemory as well, If server runs as write-heavy workloads, the data stored in memory could reach the maxmemory limit very quickly, then OOM message will be reported.
If we have maxmemory-reserved-scale parameter and client enable the lazyfree-lazy feature, for example, we can set maxmemory-reserved-scale as 0.8, it means key eviction begin when used memory reaches 8GB if maxmemory is 10GB, thus, server could continue process clients data and OOM will not happen at this time.
Thus, we can see the benefit is we can delay OOM time, but we need pay for less memory available.
One example for this paramter:
Assume
maxmemory 4GB
maxmemory-reserved-scale 20
Then we could check the detail by info memory command:
maxmemory:4294967296
maxmemory_human:4.00G
maxmemory_policy:allkeys-lru
maxmemory_reserved_scale:20
maxmemory_available:3435973836
maxmemory_available_human:3.20G
We could also update and get the maxmemory-reserved-scale value during runtime as following:
config set maxmemory-reserved-scale value
config get maxmemory-reserved-scale