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 <ranshid@amazon.com>
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 <ranshid@amazon.com>
Signed-off-by: ranshid <ranshid@amazon.com>
Signed-off-by: ranshid <ranshid@amazon.com>
Signed-off-by: ranshid <ranshid@amazon.com>
Signed-off-by: ranshid <ranshid@amazon.com>
Signed-off-by: ranshid <ranshid@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1303 +/- ##
============================================
+ Coverage 70.71% 70.87% +0.15%
============================================
Files 119 119
Lines 64652 64616 -36
============================================
+ Hits 45717 45794 +77
+ Misses 18935 18822 -113
|
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
|
I think we'll want to merge this PR first btw, #1242, since it already exists. I think there will be some merge conflicts. |
madolson
left a comment
There was a problem hiding this comment.
Mostly makes sense to me. Some ideas for clarity but nothing to really change the direction.
src/defrag.c
Outdated
| @@ -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?
There was a problem hiding this comment.
I think my last change keeps the same logic and the code is cleaner. I find having the defrag reporting the fragmentation based on the config limits is fine but take a look and let me know.
|
@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. |
Thank you. just got time to circle back on this one. merging and continue where I left off |
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
|
we will probably hit issues from day 1. eg: I think it is after the changes introduced in #1242 since the db expires can be freed in the background but is passed as target in beginDefragCycle. I wonder if to invest time to fix it now or address them later |
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
madolson
left a comment
There was a problem hiding this comment.
Direction looks fine to me, but just a minor comment about removing one of the test permutations.
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com>
…ests Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…hen allocator is not jemalloc (valkey-io#1303) 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: valkey-io#1241 --------- Signed-off-by: ranshid <ranshid@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
…vedefrag when allocator is not jemalloc (#14326) This PR is based on valkey-io/valkey#1303 This PR introduces a DEBUG_DEFRAG compilation option that enables activedefrag functionality even when the allocator is not jemalloc, and always forces defragmentation regardless of the amount or ratio of fragmentation. ## Using ``` make SANITIZER=address DEBUG_DEFRAG=<force|fully> ./runtest --debug-defrag ``` * DEBUG_DEFRAG=force * Ignore the threshold for defragmentation to ensure that defragmentation is always triggered. * Always reallocate pointers to probe for correctness issues in pointer reallocation. * DEBUG_DEFRAG=fully * Includes everything in the option `force`. * Additionally performs a full defrag on every defrag cycle, which is significantly slower but more accurate. --------- Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: oranagra <oran@redislabs.com>
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