Introduce FORCE_DEFRAG compilation option to allow activedefrag run when allocator is not jemalloc #1303
Conversation
Introduce compile time option to force activedefrag to run even when jemalloc is not used as the allocator. This is in order to be able to run tests with defrag enabled while using memory instrumantation tools. Signed-off-by: ranshid <[email protected]>
Introduce compile time option to force activedefrag to run even when jemalloc is not used as the allocator. This is in order to be able to run tests with defrag enabled while using memory instrumantation tools. Signed-off-by: ranshid <[email protected]>
Signed-off-by: ranshid <[email protected]>
Signed-off-by: ranshid <[email protected]>
Signed-off-by: ranshid <[email protected]>
Signed-off-by: ranshid <[email protected]>
Signed-off-by: ranshid <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1303 +/- ##
============================================
+ Coverage 70.69% 70.71% +0.02%
============================================
Files 115 115
Lines 63153 63153
============================================
+ Hits 44643 44661 +18
+ Misses 18510 18492 -18
|
Signed-off-by: Ran Shidlansik <[email protected]>
|
I think we'll want to merge this PR first btw, #1242, since it already exists. I think there will be some merge conflicts. |
| @@ -41,3 +41,4 @@ unset(BUILD_UNIT_TESTS CACHE) | |||
| unset(BUILD_TEST_MODULES CACHE) | |||
| unset(BUILD_EXAMPLE_MODULES CACHE) | |||
| unset(USE_TLS CACHE) | |||
| unset(FORCE_DEFRAG CACHE) | |||
There was a problem hiding this comment.
We should add this to some CI presumably. The daily ASAN probably?
There was a problem hiding this comment.
Yes I agree. maybe it can also be run as part of the CI, depending on the extra time it would take.
| message(STATUS "Forcing Active Defrag run on valkey-server") | ||
| target_compile_definitions(valkey-server PRIVATE FORCE_DEFRAG) |
There was a problem hiding this comment.
I would prefer a slightly more verbose and descriptive name, like, "DEBUG_FORCE_DEFRAG" or something, to indicate this isn't really used for production.
| @@ -755,6 +755,15 @@ void defragScanCallback(void *privdata, const dictEntry *de) { | |||
| * or not, a false detection can cause the defragmenter to waste a lot of CPU | |||
| * without the possibility of getting any results. */ | |||
| float getAllocatorFragmentation(size_t *out_frag_bytes) { | |||
There was a problem hiding this comment.
Kind of feels like you should override computeDefragCycles instead, and have that should always set active_defrag_running to like 100%.
There was a problem hiding this comment.
I am fine with it (makes sense) I do not, however want to break tests which are checking the active_defrag_running, so maybe I can just use the active-defrag-cycle-max as the return value?
Alternatively we can have a tag to skip tests in case of DEBUG_FORCE_DEFRAG
There was a problem hiding this comment.
Alternatively we can have a tag to skip tests in case of DEBUG_FORCE_DEFRAG
This intuitively makes a bit more sense to me, since defrag shouldn't really work correctly since we are completely breaking defrag here.
There was a problem hiding this comment.
All of the tests already are checking for jemalloc memory to enable the defragmentation, so I think that the tests should all still be skipped anyways?
| @@ -956,7 +966,7 @@ void activeDefragCycle(void) { | |||
| mstime_t latency; | |||
| int all_stages_finished = 0; | |||
| int quit = 0; | |||
|
|
|||
| #if !defined(FORCE_DEFRAG) | |||
There was a problem hiding this comment.
I still kind of think you should have to turn defrag on. I'm okay with overriding it so it always runs if you do, but it it's sort of counter intuitive to comment out this block of code.
There was a problem hiding this comment.
That was my initial thought as well but I went all the way :)
an alternative would be to make the active defrag config on by default when this compilation flag is on. WDYT?
There was a problem hiding this comment.
an alternative would be to make the active defrag config on by default when this compilation flag is on. WDYT?
I'm okay with this, it makes sense to me.
|
@ranshid Zvis change is now merged. Jim's is basically also ready to go so you should be able to resume updating this if you want. |
| @@ -755,6 +755,15 @@ void defragScanCallback(void *privdata, const dictEntry *de) { | |||
| * or not, a false detection can cause the defragmenter to waste a lot of CPU | |||
| * without the possibility of getting any results. */ | |||
| float getAllocatorFragmentation(size_t *out_frag_bytes) { | |||
There was a problem hiding this comment.
Can the linker feature --wrap (used via gcc with -Wl,--wrap) be leveraged to mock getAllocatorFragmentation and get_defrag_hint, providing a less invasive and cleaner solution for testing purposes?
Introduce compile time option to force activedefrag to run even when
jemalloc is not used as the allocator.
This is in order to be able to run tests with defrag enabled
while using memory instrumentation tools.
fixes: #1241