-
Notifications
You must be signed in to change notification settings - Fork 693
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
Add support for compiling with mimalloc #363
base: unstable
Are you sure you want to change the base?
Conversation
6ef673d
to
c05ecf5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #363 +/- ##
============================================
- Coverage 68.44% 68.44% -0.01%
============================================
Files 109 109
Lines 61671 61672 +1
============================================
- Hits 42212 42209 -3
- Misses 19459 19463 +4
|
Thanks for your feedback and directing me to the devendoring discussion. I will try to adjust the build scripts to link against a system-wide installation, managing |
Add support to optionally use system libraries instead of the vendored mimalloc. |
I think we should also perform certain benchmark(s) to determine the memory usage/fragmentation/throughput while using mimalloc compared to jemalloc. Will help the community understand when to use mimalloc over jemalloc for Valkey. |
I see that this PR allows the user to choose between vendoring and not. Any reason to NOT make the lib the only option? |
I agree with making the lib as the only option for the devendoring:). I will make changes to keep only one system lib opion and also run some benchmark to compare with jemalloc's memory usage/fragmentation/throughput |
Renamed below procedures and variables (missed in valkey-io#287) as follows. redis_cluster -> valkey_cluster redis1 -> valkey1 redis2 -> valkey2 get_redis_dir -> get_valkey_dir test_redis_cli_rdb_dump -> test_valkey_cli_rdb_dump test_redis_cli_repl -> test_valkey_cli_repl redis-cli -> valkey-cli redis_reset_state -> valkey_reset_state redisHandle -> valkeyHandle redis_safe_read -> valkey_safe_read redis_safe_gets -> valkey_safe_gets redis_write -> valkey_write redis_read_reply -> valkey_read_reply redis_readable -> valkey_readable redis_readnl -> valkey_readnl redis_writenl -> valkey_writenl redis_read_map -> valkey_read_map redis_read_line -> valkey_read_line redis_read_null -> valkey_read_null redis_read_bool -> valkey_read_bool redis_read_double -> valkey_read_double redis_read_verbatim_str -> valkey_read_verbatim_str redis_call_callback -> valkey_call_callback --------- Signed-off-by: Shivshankar-Reddy <[email protected]> Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Includes some more changes e.g. the README under tests and some script under utils. Signed-off-by: hwware <[email protected]> Signed-off-by: Sher Sun <[email protected]>
Add License Copyright description for Valkey contributors in COPYING --------- Signed-off-by: PatrickJS <[email protected]> Signed-off-by: Madelyn Olson <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: Sher Sun <[email protected]>
Fixes valkey-io#352 Signed-off-by: hwware <[email protected]> Signed-off-by: Sher Sun <[email protected]>
it's a supplementary modification from valkey-io#352. Signed-off-by: artikell <[email protected]> Signed-off-by: Sher Sun <[email protected]>
) This PR covers below cases. 1. test suite's prints(i.e., puts statement logs). 2. Links refering to redis issues. 3. test names contains redis. Signed-off-by: Shivshankar-Reddy <[email protected]> Signed-off-by: Sher Sun <[email protected]>
…#294) This PR has mainly done three things: 1. Enable `accept4()` on DragonFlyBSD 2. Fix the failures of determining the presence of `accept4()` due to the missing <sys/param.h> on two OSs: NetBSD, OpenBSD 3. Drop the support of FreeBSD < 10.0 for `valkey` ### References - [param.h in DragonFlyBSD](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/7485684fa5c3fadb6c7a1da0d8bb6ea5da4e0f2f/sys/sys/param.h#L129-L257) - [param.h in FreeBSD](https://github.com/freebsd/freebsd-src/blob/main/sys/sys/param.h#L46-L76) - [param.h in NetBSD](https://github.com/NetBSD/src/blob/b5f8d2f930b7ef226d4dc1b4f7017e998c0e5cde/sys/sys/param.h#L53-L70) - [param.h in OpenBSD](https://github.com/openbsd/src/blob/d9c286e032a7cee9baaecdd54eb0d43f658ae696/sys/sys/param.h#L40-L45) --------- Signed-off-by: Andy Pan <[email protected]> Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
486d12f
to
c4b890a
Compare
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Performance Test: mimalloc vs. jemalloc Install the library and header files of mimalloc:
Build the Valkey
Start cluster
valkey benchmark used: Benchmark Results:
Jemalloc logs:paas@dsde05:~/sher/valkey/src$ ./valkey-benchmark -t set -n 10000000 -c 20 -p 30003 Latency by percentile distribution: Cumulative distribution of latencies: Summary: paas@dsde05:~/sher/valkey/src$ ./valkey-cli -p 30003 mimalloc logs:paas@dsde05:~/sher/valkey/src$ ./valkey-benchmark -t set -n 10000000 -c 20 -p 30003 Latency by percentile distribution: Cumulative distribution of latencies: Summary: paas@dsde05:~/sher/valkey/src$ ./valkey-cli -p 30003 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will also need to update the README.md file as well and include the install instructions for mimalloc.
src/zmalloc.h
Outdated
#elif defined(USE_MIMALLOC) | ||
#define ZMALLOC_LIB ("mimalloc-" __xstr(MI_VERSION_MAJOR) "." __xstr(MI_VERSION_MINOR) "." __xstr(MI_VERSION_PATCH)) | ||
#include <mimalloc.h> | ||
#if (MI_VERSION_MAJOR == 1 && MI_VERSION_MINOR >= 8) || (MI_VERSION_MAJOR > 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic doesn't seem right to me. This is checking the hard-coded version numbers from L37-39. Shouldn't we check the version number defined in <mimalloc.h>?
#define MI_MALLOC_VERSION 185 // major + 2 digits minor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we sould use the MI_MALLOC_VERSION to check the version rather than hardcode value. Updated in zmalloc.h:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What happens with fragmentation on mimalloc allocator?
- Should we introduce a
defrag_supported
field toINFO
command? It would be handy for users to figure out if the server supports it or not. Currently I think settingCONFIG SET ACTIVEDEFRAG YES
would have no effect on the server.
Signed-off-by: Sher Sun <[email protected]>
…nto mimalloc-support
Mimalloc efficiently manages fragmentation through strategies like segment reuse and object migration. However, unlike jemalloc, it does not support manual defragmentation commands. |
Sure, It would clearly indicate whether active defragmentation is supported, preventing confusion. I will add this to |
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Came across this discussion on mimalloc microsoft/mimalloc#632. Looks like Dragonfly folks have made some changes by vendoring mimalloc and improving the utilization. @PingXie What are your thoughts? |
src/zmalloc.h
Outdated
@@ -47,6 +48,7 @@ | |||
|
|||
#elif defined(USE_JEMALLOC) | |||
#define ZMALLOC_LIB ("jemalloc-" __xstr(JEMALLOC_VERSION_MAJOR) "." __xstr(JEMALLOC_VERSION_MINOR) "." __xstr(JEMALLOC_VERSION_BUGFIX)) | |||
#define defrag_supported "yes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather simply compare the allocator, if it's JEMALLOC print yes else no for now. Won't require us to add another macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good suggestion:) I will update in server.c file: directly check which memory allocator is being used, avoids introducing new macros.
@@ -5757,7 +5757,8 @@ sds genRedisInfoString(dict *section_dict, int all_sections, int everything) { | |||
"mem_overhead_db_hashtable_rehashing:%zu\r\n", mh->overhead_db_hashtable_rehashing, | |||
"active_defrag_running:%d\r\n", server.active_defrag_running, | |||
"lazyfree_pending_objects:%zu\r\n", lazyfreeGetPendingObjectsCount(), | |||
"lazyfreed_objects:%zu\r\n", lazyfreeGetFreedObjectsCount())); | |||
"lazyfreed_objects:%zu\r\n", lazyfreeGetFreedObjectsCount(), | |||
"defrag_supported: %s\r\n", defrag_supported)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a tcl test to verify the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in other.tcl
file.
This is essentially the same argument for vendoring jemalloc (#364) in the first place. I think the key question is whether we believe this is going to be a common case such that the value of vendoring outweighs its overhead. I am sure fragmentation can be an issue for certain workloads but are we looking at 0.0.1%, 0.1%, 1%, or 10%? Also how bad a more generic defrag solution would be? Does there exist a generic solution that works for all allocators and gives us say 90% of "effectiveness", or 80%, 70%, etc? Vendoring itself is a concern to me and vendoring another dependency to the already long list of vendored dependencies is another concern to me. Our default stance should be "no vendoring". Exceptions are inevitable but there needs to be a very compelling argument IMO. |
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
Signed-off-by: Sher Sun <[email protected]>
f135314
to
2466cb4
Compare
@WM0323 , I'm curious to get your thoughts on what maybe some the reasons that the rust community reached the opposite conclusion - jemalloc is faster and consumes less memory than mimalloc. See discussion To clarify, not suggesting that this change shouldn't be accepted. Maybe there is something that we can learn around conducting such tests. |
Some thoughts around improving the benchmark signal:
|
Thanks for your input @yairgott . This change just allows users to use |
Signed-off-by: Sher Sun <[email protected]>
Related Issue: #346
Use mimalloc as an option when building as specified by make
MALLOC=mimalloc
orUSE_MIMALLOC=yes