From 2eb9b19612db39b1bb91902c9669e28883bbbee7 Mon Sep 17 00:00:00 2001 From: yoav-steinberg Date: Fri, 11 Feb 2022 21:58:05 +0200 Subject: [PATCH] Fix Eval scripts defrag (broken 7.0 in RC1) (#10271) Remove scripts defragger since it was broken since #10126 (released in 7.0 RC1). would crash the server if defragger starts in a server that contains eval scripts. In #10126 the global `lua_script` dict became a dict to a custom `luaScript` struct with an internal `robj` in it instead of a generic `sds` -> `robj` dict. This means we need custom code to defrag it and since scripts should never really cause much fragmentation it makes more sense to simply remove the defrag code for scripts. --- src/defrag.c | 28 +++++++++++- src/eval.c | 7 +-- src/server.h | 5 +++ tests/unit/memefficiency.tcl | 82 ++++++++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/defrag.c b/src/defrag.c index 16b1a4d2d2..b0ae487f37 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -128,6 +128,27 @@ robj *activeDefragStringOb(robj* ob, long *defragged) { return ret; } +/* Defrag helper for lua scripts + * + * returns NULL in case the allocation wasn't moved. + * when it returns a non-null value, the old pointer was already released + * and should NOT be accessed. */ +luaScript *activeDefragLuaScript(luaScript *script, long *defragged) { + luaScript *ret = NULL; + + /* try to defrag script struct */ + if ((ret = activeDefragAlloc(script))) { + script = ret; + (*defragged)++; + } + + /* try to defrag actual script object */ + robj *ob = activeDefragStringOb(script->body, defragged); + if (ob) script->body = ob; + + return ret; +} + /* Defrag helper for dictEntries to be used during dict iteration (called on * each step). Returns a stat of how many pointers were moved. */ long dictIterDefragEntry(dictIterator *iter) { @@ -256,6 +277,7 @@ long activeDefragZsetEntry(zset *zs, dictEntry *de) { #define DEFRAG_SDS_DICT_VAL_IS_SDS 1 #define DEFRAG_SDS_DICT_VAL_IS_STROB 2 #define DEFRAG_SDS_DICT_VAL_VOID_PTR 3 +#define DEFRAG_SDS_DICT_VAL_LUA_SCRIPT 4 /* Defrag a dict with sds key and optional value (either ptr, sds or robj string) */ long activeDefragSdsDict(dict* d, int val_type) { @@ -280,6 +302,10 @@ long activeDefragSdsDict(dict* d, int val_type) { void *newptr, *ptr = dictGetVal(de); if ((newptr = activeDefragAlloc(ptr))) de->v.val = newptr, defragged++; + } else if (val_type == DEFRAG_SDS_DICT_VAL_LUA_SCRIPT) { + void *newptr, *ptr = dictGetVal(de); + if ((newptr = activeDefragLuaScript(ptr, &defragged))) + de->v.val = newptr; } defragged += dictIterDefragEntry(di); } @@ -939,7 +965,7 @@ long defragOtherGlobals() { /* there are many more pointers to defrag (e.g. client argv, output / aof buffers, etc. * but we assume most of these are short lived, we only need to defrag allocations * that remain static for a long time */ - defragged += activeDefragSdsDict(evalScriptsDict(), DEFRAG_SDS_DICT_VAL_IS_STROB); + defragged += activeDefragSdsDict(evalScriptsDict(), DEFRAG_SDS_DICT_VAL_LUA_SCRIPT); defragged += moduleDefragGlobals(); return defragged; } diff --git a/src/eval.c b/src/eval.c index 41dc3b611e..c51fd22148 100644 --- a/src/eval.c +++ b/src/eval.c @@ -47,11 +47,6 @@ void ldbEnable(client *c); void evalGenericCommandWithDebugging(client *c, int evalsha); sds ldbCatStackValue(sds s, lua_State *lua, int idx); -typedef struct luaScript { - uint64_t flags; - robj *body; -} luaScript; - static void dictLuaScriptDestructor(dict *d, void *val) { UNUSED(d); if (val == NULL) return; /* Lazy freeing will set value to NULL. */ @@ -63,7 +58,7 @@ static uint64_t dictStrCaseHash(const void *key) { return dictGenCaseHashFunction((unsigned char*)key, strlen((char*)key)); } -/* server.lua_scripts sha (as sds string) -> scripts (as robj) cache. */ +/* server.lua_scripts sha (as sds string) -> scripts (as luaScript) cache. */ dictType shaScriptObjectDictType = { dictStrCaseHash, /* hash function */ NULL, /* key dup */ diff --git a/src/server.h b/src/server.h index 8c25d006ba..994e98c32c 100644 --- a/src/server.h +++ b/src/server.h @@ -3066,6 +3066,11 @@ unsigned long evalMemory(); dict* evalScriptsDict(); unsigned long evalScriptsMemory(); +typedef struct luaScript { + uint64_t flags; + robj *body; +} luaScript; + /* Blocked clients */ void processUnblockedClients(void); void blockClient(client *c, int btype); diff --git a/tests/unit/memefficiency.tcl b/tests/unit/memefficiency.tcl index 299dd658b0..55b2d6f50a 100644 --- a/tests/unit/memefficiency.tcl +++ b/tests/unit/memefficiency.tcl @@ -157,6 +157,88 @@ start_server {tags {"defrag external:skip"} overrides {appendonly yes auto-aof-r } r config set appendonly no r config set key-load-delay 0 + + test "Active defrag eval scripts" { + r flushdb + r script flush sync + r config resetstat + r config set hz 100 + r config set activedefrag no + r config set active-defrag-threshold-lower 5 + r config set active-defrag-cycle-min 65 + r config set active-defrag-cycle-max 75 + r config set active-defrag-ignore-bytes 300kb + r config set maxmemory 0 + + set n 50000 + + # Populate memory with interleaving script-key pattern of same size + set dummy_script "--[string repeat x 200]\nreturn " + set rd [redis_deferring_client] + for {set j 0} {$j < $n} {incr j} { + set val "$dummy_script[format "%06d" $j]" + $rd script load $val + $rd set k$j $val + } + for {set j 0} {$j < $n} {incr j} { + $rd read ; # Discard script load replies + $rd read ; # Discard set replies + } + after 120 ;# serverCron only updates the info once in 100ms + if {$::verbose} { + puts "used [s allocator_allocated]" + puts "rss [s allocator_active]" + puts "frag [s allocator_frag_ratio]" + puts "frag_bytes [s allocator_frag_bytes]" + } + assert_lessthan [s allocator_frag_ratio] 1.05 + + # Delete all the keys to create fragmentation + for {set j 0} {$j < $n} {incr j} { $rd del k$j } + for {set j 0} {$j < $n} {incr j} { $rd read } ; # Discard del replies + $rd close + after 120 ;# serverCron only updates the info once in 100ms + if {$::verbose} { + puts "used [s allocator_allocated]" + puts "rss [s allocator_active]" + puts "frag [s allocator_frag_ratio]" + puts "frag_bytes [s allocator_frag_bytes]" + } + assert_morethan [s allocator_frag_ratio] 1.4 + + catch {r config set activedefrag yes} e + if {[r config get activedefrag] eq "activedefrag yes"} { + + # wait for the active defrag to start working (decision once a second) + wait_for_condition 50 100 { + [s active_defrag_running] ne 0 + } else { + fail "defrag not started." + } + + # wait for the active defrag to stop working + wait_for_condition 500 100 { + [s active_defrag_running] eq 0 + } else { + after 120 ;# serverCron only updates the info once in 100ms + puts [r info memory] + puts [r memory malloc-stats] + fail "defrag didn't stop." + } + + # test the the fragmentation is lower + after 120 ;# serverCron only updates the info once in 100ms + if {$::verbose} { + puts "used [s allocator_allocated]" + puts "rss [s allocator_active]" + puts "frag [s allocator_frag_ratio]" + puts "frag_bytes [s allocator_frag_bytes]" + } + assert_lessthan_equal [s allocator_frag_ratio] 1.05 + } + # Flush all script to make sure we don't crash after defragging them + r script flush sync + } {OK} test "Active defrag big keys" { r flushdb