-
Notifications
You must be signed in to change notification settings - Fork 636
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 CMake build system for valkey #1082
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1082 +/- ##
============================================
- Coverage 70.69% 70.69% -0.01%
============================================
Files 114 114
Lines 63076 63076
============================================
- Hits 44594 44589 -5
- Misses 18482 18487 +5
|
Thanks @eifrah-aws! I like this change but I think we need a @valkey-io/core-team discussion first. |
@PingXie, thanks for looking into this. In the meanwhile, I will continue adding the missing functionalities that I want to add to Another approach is to let both build system co-exist, until a point in time where you (and the core team) feels that its matured enough and a switch can be made (i.e. deprecate the old |
This was previously discussed here: #79. The original decision was very mixed which was why I don't think we originally moved forward with it. Reading it through again, I didn't find the arguments very compelling given that our system is working well but don't feel strongly. |
I don't like this approach, it just means we have to pay all the cost of both system without getting the benefits of CMAKE. We should either switch or stick with MAKE. If we find something that doesn't work well, we can always just revert the migration to CMAKE. We do have the compatibility story to deal with, since it will be adding a new dependency that users will need to have available. I can't imagine in this day it will be much of a problem. |
The way I see it: the end goal is to switch to
I was not aware of this discussion - thanks for bringing it up. Reading the OP arguments - they are basically the same as mine: parallel builds & out of source tree builds In addition, tooling is important any developer that worked with VScode know the frustration of not having the IDE code completing when typing (or find references etc) CMake has a built-in support for |
The current build system works but doesn't work well. We added band-aids like Line 55 in bb57dfe
The one argument against CMake that is worth some discussion IMO is "dependency". I believe @zuiderkwast brought it up in the past that some organizations might not have CMake in their tool chains so this switch could be considered disruptive, even temporarily? I haven't seen a concrete case personally though. I also don't think the value of supporting two build systems outweighs the (complexity) overhead. If we decide to switch, we should fully commit to it. |
This is probably a situation where we will have to just make the change and see what breaks. We didn't know that folks didn't have support for C11 until we switched over and people brought up that it was broken. I added the run extra test flag so we'll at least run it on all the different containers we think about. |
The current system does work though, so ultimately it's a style preference, which I don't find compelling to make a change that requires work from packagers and will break existing developers.
I really feel unsure on this. We would have to make this for 9.0, and do we really want to force packagers to do work for it? I like the out of tree builds and better integration (this seems like the biggest win imo). If the goal is better integration, I would live with supporting both. So, I guess let's finish creating a fully featured CMAKE file and see how much effort it is to maintain? |
If this PR is accepted, I am willing to maintain it |
That's not how it works. Whenever someone changes something like a compiler flag or some variable in a PR, they have to update the Makefile and CMakeLists.txt in that same PR. Are you suggesting we drop the Makefile or should we support two build systems in parallel? Just like I don't like to switch to C++ for the source code, I don't want to switch to CMake for the builds. It adds complexity and hides what's actually happening, which is something that Make doesn't, besides multiple of us maintainers have expressed that we know Make very well and are not familiar with CMake. I believe it is important that the maintainers are comfortable with the tools we're using. I was very happy that the discussion in #79 ended in April, so we could focus on developing instead of debating build systems. Now we have to spend time on that again.
This is the only one of the "pros" I think is compelling. (Auto-completion is an anti-feature IMHO. If you think you need it, it means that the source code is too complex.) Some more cons already mentioned in #79 are complexity and implicit behaviour. Risk that building with make and cmake don't give the same results (due to implicit changes of compilation flags like -std=c11 to -std=gnu11 that cmake does). The way we optionally regenerate some files like commands.def that are checked into the repo is easy to achieve with make. I don't think we would have bothered to implement this if we'd be on CMake. |
Eventually, I would like replace Make with CMake
This statement it subjective. I argue that CMake simplify things
Sorry about that
Here is the output from valkey macOS build using [ 75%] Building C object src/CMakeFiles/valkey-server.dir/server.c.o
cd /Users/eifrah/devl/valkey/build-release/src && /usr/bin/gcc -I/Users/eifrah/devl/valkey/deps/hiredis -I/Users/eifrah/devl/valkey/deps/linenoise -I/Users/eifrah/devl/valkey/deps/lua/src -I/Users/eifrah/devl/valkey/deps/hdr_histogram -I/Users/eifrah/devl/valkey/deps/fpconv -pedantic -O3 -DNDEBUG -std=c11 -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk -mmacosx-version-min=14.7 -MD -MT src/CMakeFiles/valkey-server.dir/server.c.o -MF CMakeFiles/valkey-server.dir/server.c.o.d -o CMakeFiles/valkey-server.dir/server.c.o -c /Users/eifrah/devl/valkey/src/server.c
Auto-Completion is not only "display possible completions", it is a complete tool set that increases developer productivity and allows him/her to focus on what is important: coding. A good example is: refactor symbol: sure, I can do that by doing grep and then manually replace what is correct (based on the context), or I can "right click -> rename" and click OK... |
Complexity and simplicity are not subjective terms. Complexity can be analyzed. So can explicit vs implicit. Easy and difficult are subject terms. Easy is what you are familiar with and already know, while simple is the lack of complexities such as logical dependencies between things. (There's a really nice talk about this by Rich Hickey, the inventor of Clojure: https://www.youtube.com/watch?v=SxdOUGdseq4) Make has very few pre-defined rules (like .o depending on .c) while CMake has a lot of such pre-defined rules. The reason things often just work in CMake is that these things are pre-defined (which is good when it does what you want and not good when it doesn't), whereas with make you have to write all the rules explicitly. CMake generates makefiles and other kinds of files. Then you run these makefiles using make, or ninja files using ninja, etc. There's always a risk that there are differences in these different targets, since the tools are different in various ways. (The generated makefiles are perhaps not for human consumption, but to find out what cmake does, I have tried reading them in the past without much luck. :D).
Yes, I see that. I believe it does change this by default though and fortunately you knew that we have to add I do think CMake is very capable of a lot of things. I just don't see the need for it in this project. Changing things imply a friction both to users and developers. It would be more justified to add CMake if we would have either of these "problems":
|
One thing to keep in mind is that CMake is actively developed and new features are coming every now and then. There are feature in CMake that I have liked in other projects though, like out-of-source builds and being able to generate a ninja build which are often a bit faster, but its has a maintenance cost. |
@bjosv I believe that this is addressable by using |
Let us consider the following criteria, if CMAKE over MAKE more advantages, we can commit it:
|
According to my experience:
|
For example, if I had a to write a simple hello_world: hello.o
g++ -o hello_world hello.o
hello.o: hello.cpp
g++ -c hello.cpp -o hello.o
clean:
rm *.o hello_world vs add_executable(hello_world "hello.cpp") It only gets complicated. Imagine that you want to add openssl, then you need to know other tools like find_package(OpenSSL REQUIRED)
include_directories(${OPENSSL_INCLUDE_DIR})
target_link_libraries(hello_world OpenSSL::SSL) And this will work for Windows (MSVC), Linux, SunOS, FreeBSD, MinGW and other platforms so, IMHO,
What I mentioned earlier:
|
With this CMake line, I have to go read the documentation to understand add_executable call. When I have to modify something, I need to figure out how to modify "add_executable". And if "add_executable" does not support it then? Is this really better? |
CMake has an excellent documentation + great search result in Stackoverflow
Anything that can be done with But don't trust my word on that, many complex projects are using To list some from the top of my head:
Obviously, the list is too extensive to cover. But my point is: if all of these projects are using CMake without an issue, we can do it too.
IMO, yes. BTW: you can always run the generated Makefile with |
We had a conversation around the same aspect internally and it's not just about organizations IMHO, it's also about a new developer trying to hack on valkey. We don't want to disrupt that experience and facing challenges around setting up Cmake (which Cmake) in the first place. This is where the project shined for me personally where you can just download the source and build it. No dependencies altogether. |
@hpatro not possible.
|
Ci report this failure: ``` *** [err]: SHUTDOWN NOSAVE can kill a timedout script anyway in tests/unit/scripting.tcl Expected 'BUSY Valkey is busy running a script. *' to match '*connection refused*' (context: type eval line 8 cmd {assert_match {*connection refused*} $e} proc ::test) ``` We can see the logs the shutdown got rejected because there is an AOFRW pending: ``` Writing initial AOF, can't exit. Errors trying to shut down the server. Check the logs for more information. ``` The reason is that the previous test enabled the aof. Signed-off-by: Binbin <[email protected]>
Typo, comment cleanups. Signed-off-by: Binbin <[email protected]>
in case of valgrind run, the replica might get disconnected from the primary due to repl-timeout reached. Fix is to configure larger timeout in case of valgrind test. **Partially** fixes: valkey-io#1152 Signed-off-by: Ran Shidlansik <[email protected]>
…el-replication (valkey-io#1164) Sometimes when dual-channel is turned off the tested replica might disconnect on COB overrun. disable the replica COB limit in order to prevent such cases. Fixes: valkey-io#1153 Signed-off-by: Ran Shidlansik <[email protected]> Signed-off-by: Binbin <[email protected]> Co-authored-by: Binbin <[email protected]>
Currently in our daily, if a job fails, it will cancel the other jobs in the same matrix, we want to avoid this so that all jobs in a matrix can eventually run to completion. Docs: jobs.<job_id>.strategy.fail-fast applies to the entire matrix. If jobs.<job_id>.strategy.fail-fast is set to true or its expression evaluates to true, GitHub will cancel all in-progress and queued jobs in the matrix if any job in the matrix fails. This property defaults to true. Signed-off-by: Binbin <[email protected]>
Minor cleanup, listLast do the same thing and is widely used and easier to understand (less code). Signed-off-by: Binbin <[email protected]>
1. Make sure to assert the ERR prefix. 2. Match "Syntax error*" in case of the message change. Signed-off-by: Binbin <[email protected]>
…oc,free} (valkey-io#1169) The zcalloc symbol is a symbol name already used by zlib, which is defining other names using the "z" prefix specific to zlib. In practice, linking valkey with a static openssl, which itself might depend on a static libz will result in link time error rejecting multiple symbol definitions. Fixes: valkey-io#1157 Signed-off-by: Romain Geissler <[email protected]>
…o#977) Currently in conf we describe activerehashing as: Active rehashing uses 1 millisecond every 100 milliseconds of CPU time. This is the case for hz = 10. If we change hz, the description in conf will be inaccurate. Users may notice that the server spends some CPU (used in activerehashing) at high hz but don't know why, since our cron calls are fixed to 1ms. This PR takes hz into account and fixed the CPU usage at 1% (this may not be accurate in some cases because we do 100 step rehashing in dictRehashMicroseconds but it can avoid CPU spikes in this case). This PR also improves the description of the activerehashing configuration item to explain this change. Signed-off-by: Binbin <[email protected]> Co-authored-by: Viktor Söderqvist <[email protected]>
…y-io#1171) The client that was killed by FUNCTION KILL received a reply of SCRIPT KILL and the server log also showed SCRIPT KILL. Signed-off-by: Binbin <[email protected]>
Consolidate the cleanup of local variables to a single point within the method, ensuring proper resource management and p reventing memory leaks or double-free issues. Previoslly descused here: - valkey-io#60 (comment) - valkey-io#60 (comment) --------- Signed-off-by: naglera <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Co-authored-by: Ping Xie <[email protected]>
Remove reference to "the mailing list". We don't have a mailing list.
**Overview** This PR introduces the use of [MurmurHash3](https://en.wikipedia.org/wiki/MurmurHash) as the hashing function for Lua's luaS_newlstr function, replacing the previous simple hash function. The change aims to improve performance, particularly for large strings. **Changes** Implemented MurmurHash3 algorithm in lstring.c Updated luaS_newlstr to use MurmurHash3 for string hashing **Performance Testing:** Test Setup: 1. Ran a valkey server 2. Loaded 1000 keys with large values (100KB each) to the server using a Lua script ``` local numKeys = 1000 for i = 1, numKeys do local key = "large_key_" .. i local largeValue = string.rep("x", 1024*100) redis.call("SET", key, largeValue) end ``` 3. Used a Lua script to randomly select and retrieve keys ``` local randomKey = redis.call("RANDOMKEY") local result = redis.call("GET", randomKey) ``` 4. Benchmarked using valkey-benchmark: `./valkey-benchmark -n 100000 evalsha c157a37967e69569339a39a953c046fc2ecb4258 0` Results: A | Unstable | This PR | Change -- | -- | -- | -- Throughput | 6,835.74 requests per second | 17,061.94 requests per second | **+150% increase** Avg Latency | 7.218 ms | 2.838 ms | **-61% decrease** Min Latency | 3.144 ms | 1.320 ms | **-58% decrease** P50 Latency | 8.463 ms | 3.167 ms | **-63% decrease** P95 Latency | 8.863 ms | 3.527 ms | **-60% decrease** P99 Latency | 9.063 ms | 3.663 ms | **-60% decrease** Max Latency | 63.871 ms | 55.327 ms | **-13% decrease** Summary: * Throughput: Improved by 150%. * Latency: Significant reductions in average, minimum, and percentile latencies (P50, P95, P99), leading to much faster response times. * Max Latency: Slightly decreased by 13%, indicating fewer outlier delays after the fix. --------- Signed-off-by: Shai Zarka <[email protected]> Signed-off-by: zarkash-aws <[email protected]> Signed-off-by: Madelyn Olson <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Viktor Söderqvist <[email protected]>
…o#1155) valkey-io#1145 First part of a two-step effort to add `WithSlot` API for expiry. This PR is to fix a crash that occurs when a RANDOMKEY uses a different slot than the cached slot of a client during a multi-exec. The next part will be to utilize the new API as an optimization to prevent duplicate work when calculating the slot for a key. --------- Signed-off-by: Nadav Levanoni <[email protected]> Signed-off-by: Madelyn Olson <[email protected]> Co-authored-by: Nadav Levanoni <[email protected]> Co-authored-by: Madelyn Olson <[email protected]>
…primary nodes (valkey-io#1075) There is no limitation in Valkey to create a cluster with 1 or 2 primaries, only that it cannot do automatic failover. Remove this restriction and add `are you sure` prompt to prompt the user. This allow we use it to create a test cluster by cli or by create-cluster. Signed-off-by: Binbin <[email protected]>
…s `used_memory_thread`. (valkey-io#1179) When profiling some workloads with `io-threads` enabled. We found the false sharing issue is heavy. This patch try to split the the elements accessed by main thread and io-threads into different cache line by padding the elements in the head of `used_memory_thread_padded` array. This design helps mitigate the false sharing between main thread and io-threads, because the main thread has been the bottleneck with io-threads enabled. We didn't put each element in an individual cache line is that we don't want to bring the additional cache line fetch operation (3 vs 16 cache line) when call function like `zmalloc_used_memory()`. --------- Signed-off-by: Lipeng Zhu <[email protected]> Signed-off-by: Lipeng Zhu <[email protected]> Signed-off-by: Viktor Söderqvist <[email protected]> Co-authored-by: Wangyang Guo <[email protected]> Co-authored-by: Viktor Söderqvist <[email protected]>
Hide 'unixsocketgroup' and 'unixsocketperm' into a Unix socket specific data structure. A single opaque pointer 'void *priv' is enough for a listener. Once any new config is added, we don't need 'void *priv2', 'void *priv3' and so on. Signed-off-by: zhenwei pi <[email protected]>
…y-io#1182) This special pattern '#' is used to get the element itself, it does not actually participate in the slot check. In this case, passing `GET #` will cause '#' to participate in the slot check, causing the command to get an `pattern may be in different slots` error. Signed-off-by: Binbin <[email protected]>
Signed-off-by: Eran Ifrah <[email protected]>
9b6047f
to
110d5b9
Compare
This PR LGTM overall. @eifrah-aws, please feel free to tag me when you reopen this PR and I will provide the detailed feedback. |
@PingXie I made the unfortunate mistake of clicking the "Sync Fork" button on my repo which caused a mess in the commit hisotry, so I had to re-create the branch. The unfortunate side effect was that the PR got closed... 😄 Can you re-open it? or should I recreate it (and reference this one to retain the discussion history?) |
never mind, after refreshing this page, the "Reopen and comment" button is back. |
Thanks for trying, new PR created: #1196 |
NOTE:
This PR was tested on:
gmake
for buildingvalkey
With this PR, users are able to build valkey using
CMake
.Example usage:
Build
valkey-server
in Release mode with TLS enabled and usingjemalloc
as the allocator:Build
valkey-unit-tests
:Current features supported by this PR:
jemalloc
,tcmalloc
,tcmalloc_minimal
andlibc
), e.g. to enablejemalloc
pass-DWITH_MALLOC=jemalloc
tocmake
-DWITH_TLS=1
tocmake
)-DWITH_SANITIZER=<address|thread|undefined>
tocmake
valkey-unit-tests
executablevalkey
under/home/you/root
pass-DCMAKE_INSTALL_PREFIX=/home/you/root
Why using
CMake
? To list some of the advantages of usingCMake
:compile_commands.json
which is required byclangd
to get a compiler accuracy code completion (in other words: your VScode will thank you)More build examples:
ASAN build:
ASAN with jemalloc:
As seen by the previous examples, any combination is allowed and co-exist on the same source tree.
Valkey installation
With this new
CMake
, it is possible to install the binary by runningmake install
or creating a packagemake package
(currently supported on Debian like distros)Example 1: build & install using
make install
:Example 2: create a
.deb
installer:Example 3: create installer for non Debian systems (e.g. FreeBSD or macOS):
Current
CMake
PR status:jemalloc
,libc
,tcmalloc
andtcmalloc_minimal
).deb
packagingrelease.h
commands.def
fmtargs.h
test_files.h