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

Merged
merged 25 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
5044dfc
compile time option to force activedefrag
ranshid Nov 13, 2024
c62beb2
compile time option to force activedefrag
ranshid Nov 13, 2024
31a04dc
Add Cmake support
ranshid Nov 14, 2024
a42e565
Make defrag always active when forced
ranshid Nov 14, 2024
1f26c5d
Simplify the FORCE_DEFRAG Cmake logic
ranshid Nov 14, 2024
f36ee08
order and fix some uneeded changes
ranshid Nov 14, 2024
27f4290
make changes to SERVER_FLAGS
ranshid Nov 14, 2024
c1862eb
Merge branch 'unstable', remote-tracking branch 'origin' into force-d…
ranshid Nov 14, 2024
4643d97
fix spell and format issues
ranshid Nov 14, 2024
5d65221
Merge remote-tracking branch 'origin/unstable' into force-defrag
ranshid Dec 8, 2024
1744aab
merge and fix some pr comments
ranshid Dec 9, 2024
14e102e
add daily workflow test
ranshid Dec 9, 2024
bd2b04a
fix compilation on x86
ranshid Dec 9, 2024
6606bb6
format fixes
ranshid Dec 9, 2024
ec6ad04
more formatt fighting
ranshid Dec 9, 2024
cf5841f
one last try to solve the format issue
ranshid Dec 9, 2024
329167f
format
ranshid Dec 9, 2024
05d3b03
format
ranshid Dec 9, 2024
0c9f535
Update src/zmalloc.c
madolson Dec 9, 2024
6cf641a
Update src/allocator_defrag.h
madolson Dec 9, 2024
51e83ad
Update .github/workflows/daily.yml
ranshid Dec 10, 2024
5a836a1
add debug-defrag flag for tests + debug_defarg::skip tag option for t…
ranshid Dec 10, 2024
cb1b2ce
fix bad global tag
ranshid Dec 10, 2024
0c1b7b0
preset debug_defrag global fleg
ranshid Dec 11, 2024
5ddb45e
Merge remote-tracking branch 'origin/unstable' into force-defrag
ranshid Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
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)
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?

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 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.

/* 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)
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