Skip to content
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

Introduce FORCE_DEFRAG compilation option to allow activedefrag run when allocator is not jemalloc #1303

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this to some CI presumably. The daily ASAN probably?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. maybe it can also be run as part of the CI, depending on the extra time it would take.

4 changes: 3 additions & 1 deletion deps/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
add_subdirectory(jemalloc)
if (USE_JEMALLOC)
add_subdirectory(jemalloc)
endif ()
add_subdirectory(lua)

# Set hiredis options. We need to disable the defaults set in the OPTION(..) we do this by setting them in the CACHE
Expand Down
6 changes: 6 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ if (VALKEY_RELEASE_BUILD)
set_property(TARGET valkey-server PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
endif ()

if (FORCE_DEFRAG)
message(STATUS "Forcing Active Defrag run on valkey-server")
target_compile_definitions(valkey-server PRIVATE FORCE_DEFRAG)
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

target_compile_definitions(valkey-server PRIVATE HAVE_DEFRAG)
endif ()

# Target: valkey-cli
list(APPEND CLI_LIBS "linenoise")
valkey_build_and_install_bin(valkey-cli "${VALKEY_CLI_SRCS}" "${VALKEY_SERVER_LDFLAGS}" "${CLI_LIBS}" "redis-cli")
Expand Down
5 changes: 5 additions & 0 deletions src/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ ifdef REDIS_LDFLAGS
SERVER_LDFLAGS := $(REDIS_LDFLAGS)
endif

# Special case of forcing defrag to run even though we have no Jemlloc support
ifeq ($(FORCE_DEFRAG), yes)
SERVER_CFLAGS +=-DHAVE_DEFRAG -DFORCE_DEFRAG
endif

FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(SERVER_CFLAGS)
FINAL_LDFLAGS=$(LDFLAGS) $(OPT) $(SERVER_LDFLAGS) $(DEBUG)
FINAL_LIBS=-lm
Expand Down
24 changes: 22 additions & 2 deletions src/defrag.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of feels like you should override computeDefragCycles instead, and have that should always set active_defrag_running to like 100%.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@madolson madolson Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/* In case we are forcing defrag to run without Jemalloc support we cannot get any
* good statistics from the allocator regarding external fragmentation.
* This is why we are forcing the report to reflect fragmented system conditions based on the existing configurations. */
#if defined(FORCE_DEFRAG) || !defined(USE_JEMALLOC)

*out_frag_bytes = server.active_defrag_ignore_bytes + 1;
return server.active_defrag_threshold_upper;
#else

size_t resident, active, allocated, frag_smallbins_bytes;
zmalloc_get_allocator_info(&allocated, &active, &resident, NULL, NULL, &frag_smallbins_bytes);

Expand All @@ -769,6 +778,7 @@ float getAllocatorFragmentation(size_t *out_frag_bytes) {
serverLog(LL_DEBUG, "allocated=%zu, active=%zu, resident=%zu, frag=%.2f%% (%.2f%% rss), frag_bytes=%zu (%zu rss)",
allocated, active, resident, frag_pct, rss_pct, frag_smallbins_bytes, rss_bytes);
return frag_pct;
#endif
}

/* Defrag scan callback for the pubsub dictionary. */
Expand Down Expand Up @@ -956,7 +966,7 @@ void activeDefragCycle(void) {
mstime_t latency;
int all_stages_finished = 0;
int quit = 0;

#if !defined(FORCE_DEFRAG)
Copy link
Member

@madolson madolson Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!server.active_defrag_enabled) {
if (server.active_defrag_running) {
/* if active defrag was disabled mid-run, start from fresh next time. */
Expand All @@ -975,7 +985,10 @@ void activeDefragCycle(void) {
}
return;
}

#else
/* Avoid compiler warning */
if (0) goto update_metrics;
#endif
if (hasActiveChildProcess()) return; /* Defragging memory while there's a fork will just do damage. */

/* Once a second, check if the fragmentation justfies starting a scan
Expand Down Expand Up @@ -1134,6 +1147,13 @@ void activeDefragCycle(void) {
}
}

#if defined(FORCE_DEFRAG) || !defined(JEMALLOC_FRAG_HINT)
int je_get_defrag_hint(void *ptr) {
UNUSED(ptr);
return 1;
}
#endif

#else /* HAVE_DEFRAG */

void activeDefragCycle(void) {
Expand Down
16 changes: 16 additions & 0 deletions src/zmalloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ void *zmalloc_usable(size_t size, size_t *usable) {
* and go straight to the allocator arena bins.
* Currently implemented only for jemalloc. Used for online defragmentation. */
#ifdef HAVE_DEFRAG
#if defined(USE_JEMALLOC)
void *zmalloc_no_tcache(size_t size) {
if (size >= SIZE_MAX / 2) zmalloc_oom_handler(size);
void *ptr = mallocx(size + PREFIX_SIZE, MALLOCX_TCACHE_NONE);
Expand All @@ -224,6 +225,21 @@ void zfree_no_tcache(void *ptr) {
update_zmalloc_stat_free(zmalloc_size(ptr));
dallocx(ptr, MALLOCX_TCACHE_NONE);
}
#else
void *zmalloc_no_tcache(size_t size) {
if (size >= SIZE_MAX / 2) zmalloc_oom_handler(size);
void *ptr = malloc(size + PREFIX_SIZE);
if (!ptr) zmalloc_oom_handler(size);
update_zmalloc_stat_alloc(zmalloc_size(ptr));
return ptr;
}

void zfree_no_tcache(void *ptr) {
if (ptr == NULL) return;
update_zmalloc_stat_free(zmalloc_size(ptr));
free(ptr);
}
#endif
#endif

/* Try allocating memory and zero it, and return NULL if failed.
Expand Down
Loading