Offload slot calculation and cross-slot detection to I/O threads#2165
Merged
Conversation
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2165 +/- ##
============================================
+ Coverage 71.35% 71.46% +0.11%
============================================
Files 122 122
Lines 66178 66210 +32
============================================
+ Hits 47222 47319 +97
+ Misses 18956 18891 -65
🚀 New features to boost your workflow:
|
ranshid
reviewed
Jun 3, 2025
ranshid
left a comment
Member
There was a problem hiding this comment.
Looks good, but I will probably need a 2nd look. some small comments for now
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
ranshid
reviewed
Jun 3, 2025
ranshid
left a comment
Member
There was a problem hiding this comment.
nit - just to reduce the diff little bit.
ranshid
reviewed
Jun 3, 2025
madolson
reviewed
Jun 4, 2025
madolson
approved these changes
Jun 4, 2025
madolson
left a comment
Member
There was a problem hiding this comment.
No real concerns for me, looks good
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
ranshid
approved these changes
Jun 4, 2025
zuiderkwast
commented
Jun 4, 2025
Member
|
Did some benchmarks, results are in related issue |
PingXie
reviewed
Jun 9, 2025
PingXie
left a comment
Member
There was a problem hiding this comment.
sorry for being late, again :) looks great to me - just one minor comment and I think getNodeByQuery could be further simplified. thanks @zuiderkwast
chzhoo
pushed a commit
to chzhoo/valkey
that referenced
this pull request
Jun 12, 2025
…key-io#2165) Refactors `getNodeByQuery()` to be able to move the CRC16 slot calculations in I/O threads and skip many checks if slot migrations are not ongoing. The slot calculation for a command is moved out of `getNodeByQuery()` into a new function `clusterSlotByCommand()` which is safe to call from I/O threads. For MULTI-EXEC transactions, the slot is stored per command in the multi-state, to be able to detect cross-slot transactions on EXEC without computing the slots again. Additionally, cross-slot detection and arity check is offloaded to I/O threads. To unify the code paths for commands parsed by I/O threads and commands parsed by the main thread, the command lookup, arity check, slot lookup are moved out of `processCommand()` to a new `prepareCommand()` function that needs to be called before `processCommand()`. The client's read flags are used for passing information about bad arity and cross-slot. These flags are already used to convey information per command between I/O thread and main thread. Fixes valkey-io#2077 Related to valkey-io#632 --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: chzhoo <czawyx@163.com>
shanwan1
pushed a commit
to shanwan1/valkey
that referenced
this pull request
Jun 13, 2025
…key-io#2165) Refactors `getNodeByQuery()` to be able to move the CRC16 slot calculations in I/O threads and skip many checks if slot migrations are not ongoing. The slot calculation for a command is moved out of `getNodeByQuery()` into a new function `clusterSlotByCommand()` which is safe to call from I/O threads. For MULTI-EXEC transactions, the slot is stored per command in the multi-state, to be able to detect cross-slot transactions on EXEC without computing the slots again. Additionally, cross-slot detection and arity check is offloaded to I/O threads. To unify the code paths for commands parsed by I/O threads and commands parsed by the main thread, the command lookup, arity check, slot lookup are moved out of `processCommand()` to a new `prepareCommand()` function that needs to be called before `processCommand()`. The client's read flags are used for passing information about bad arity and cross-slot. These flags are already used to convey information per command between I/O thread and main thread. Fixes valkey-io#2077 Related to valkey-io#632 --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: shanwan1 <shanwan1@intel.com>
This was referenced Oct 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors
getNodeByQuery()to be able to move the CRC16 slot calculations in I/O threads and skip many checks if slot migrations are not ongoing.The slot calculation for a command is moved out of
getNodeByQuery()into a new functionclusterSlotByCommand()which is safe to call from I/O threads.For MULTI-EXEC transactions, the slot is stored per command in the multi-state, to be able to detect cross-slot transactions on EXEC without computing the slots again.
Sorry for the large diff within
getNodeByQuery(). This function is quite long and complex. I added agototo skip parts of it, to avoid indenting a large code block. (We can do it, but it gives a larger diff to review.)Additionally, cross-slot detection and arity check is offloaded to I/O threads. To unify the code paths for commands parsed by I/O threads and commands parsed by the main thread, the command lookup, arity check, slot lookup are moved out of
processCommand()to a newprepareCommand()function that needs to be called beforeprocessCommand().The client's read flags are used for passing information about bad arity and cross-slot. These flags are already used to convey information per command between I/O thread and main thread.
Fixes #2077
Related to #632