-
Notifications
You must be signed in to change notification settings - Fork 653
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
Optimize ZUNION[STORE] command by removing unnecessary accumulator dict #829
Merged
soloestoy
merged 2 commits into
valkey-io:unstable
from
RayaCoo:remove_accumulator_dict
Aug 13, 2024
Merged
Optimize ZUNION[STORE] command by removing unnecessary accumulator dict #829
soloestoy
merged 2 commits into
valkey-io:unstable
from
RayaCoo:remove_accumulator_dict
Aug 13, 2024
Conversation
This file contains 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
Signed-off-by: RayCao <zisong.cw@alibaba-inc.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #829 +/- ##
============================================
- Coverage 70.46% 70.27% -0.20%
============================================
Files 112 112
Lines 61135 61463 +328
============================================
+ Hits 43077 43191 +114
- Misses 18058 18272 +214
|
hpatro
approved these changes
Aug 9, 2024
Signed-off-by: zisong.cw <zisong.cw@alibaba-inc.com>
soloestoy
approved these changes
Aug 13, 2024
soloestoy
added
the
release-notes
This issue should get a line item in the release notes
label
Aug 13, 2024
mapleFU
pushed a commit
to mapleFU/valkey
that referenced
this pull request
Aug 21, 2024
…ct (valkey-io#829) In the past implementation of `ZUNION` and `ZUNIONSTORE` commands, we first create a temporary dict called `accumulator`. After adding all member-score mappings to `accumulator`, we still need to convert `accumulator` back to the final dict `dstzset->dict`. However, we can directly use `dstzset->dict` to avoid the additional copy operation. This PR removes the `accumulator` dict and directly uses` dstzset->dict `to store the member-score mappings. - **Test** First, I added 300 unique elements to two sorted sets called 'zunion_test1' and 'zunion_test2'. Then, I tested `zunion` and `zunionstore` on these two sorted sets. The test results shown below indicate that the performance of both zunion and zunionstore improved about 31%. ### ZUNION #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 2713.41 requests per second latency summary (msec): avg min p50 p95 p99 max 146.252 3.464 153.343 182.015 184.959 192.895 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 3543.84 requests per second latency summary (msec): avg min p50 p95 p99 max 108.259 2.984 114.239 141.695 145.151 160.255 ``` ### ZUNIONSTORE #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 3168.07 requests per second latency summary (msec): avg min p50 p95 p99 max 157.511 3.368 183.167 189.311 193.535 231.679 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 4144.73 requests per second latency summary (msec): avg min p50 p95 p99 max 120.374 2.648 141.823 149.119 153.855 183.167 ``` --------- Signed-off-by: RayCao <zisong.cw@alibaba-inc.com> Signed-off-by: zisong.cw <zisong.cw@alibaba-inc.com> Signed-off-by: mwish <maplewish117@gmail.com>
mapleFU
pushed a commit
to mapleFU/valkey
that referenced
this pull request
Aug 22, 2024
…ct (valkey-io#829) In the past implementation of `ZUNION` and `ZUNIONSTORE` commands, we first create a temporary dict called `accumulator`. After adding all member-score mappings to `accumulator`, we still need to convert `accumulator` back to the final dict `dstzset->dict`. However, we can directly use `dstzset->dict` to avoid the additional copy operation. This PR removes the `accumulator` dict and directly uses` dstzset->dict `to store the member-score mappings. - **Test** First, I added 300 unique elements to two sorted sets called 'zunion_test1' and 'zunion_test2'. Then, I tested `zunion` and `zunionstore` on these two sorted sets. The test results shown below indicate that the performance of both zunion and zunionstore improved about 31%. ### ZUNION #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 2713.41 requests per second latency summary (msec): avg min p50 p95 p99 max 146.252 3.464 153.343 182.015 184.959 192.895 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 3543.84 requests per second latency summary (msec): avg min p50 p95 p99 max 108.259 2.984 114.239 141.695 145.151 160.255 ``` ### ZUNIONSTORE #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 3168.07 requests per second latency summary (msec): avg min p50 p95 p99 max 157.511 3.368 183.167 189.311 193.535 231.679 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 4144.73 requests per second latency summary (msec): avg min p50 p95 p99 max 120.374 2.648 141.823 149.119 153.855 183.167 ``` --------- Signed-off-by: RayCao <zisong.cw@alibaba-inc.com> Signed-off-by: zisong.cw <zisong.cw@alibaba-inc.com> Signed-off-by: mwish <maplewish117@gmail.com>
madolson
pushed a commit
that referenced
this pull request
Sep 2, 2024
…ct (#829) In the past implementation of `ZUNION` and `ZUNIONSTORE` commands, we first create a temporary dict called `accumulator`. After adding all member-score mappings to `accumulator`, we still need to convert `accumulator` back to the final dict `dstzset->dict`. However, we can directly use `dstzset->dict` to avoid the additional copy operation. This PR removes the `accumulator` dict and directly uses` dstzset->dict `to store the member-score mappings. - **Test** First, I added 300 unique elements to two sorted sets called 'zunion_test1' and 'zunion_test2'. Then, I tested `zunion` and `zunionstore` on these two sorted sets. The test results shown below indicate that the performance of both zunion and zunionstore improved about 31%. ### ZUNION #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 2713.41 requests per second latency summary (msec): avg min p50 p95 p99 max 146.252 3.464 153.343 182.015 184.959 192.895 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 3543.84 requests per second latency summary (msec): avg min p50 p95 p99 max 108.259 2.984 114.239 141.695 145.151 160.255 ``` ### ZUNIONSTORE #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 3168.07 requests per second latency summary (msec): avg min p50 p95 p99 max 157.511 3.368 183.167 189.311 193.535 231.679 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 4144.73 requests per second latency summary (msec): avg min p50 p95 p99 max 120.374 2.648 141.823 149.119 153.855 183.167 ``` --------- Signed-off-by: RayCao <zisong.cw@alibaba-inc.com> Signed-off-by: zisong.cw <zisong.cw@alibaba-inc.com>
madolson
pushed a commit
that referenced
this pull request
Sep 3, 2024
…ct (#829) In the past implementation of `ZUNION` and `ZUNIONSTORE` commands, we first create a temporary dict called `accumulator`. After adding all member-score mappings to `accumulator`, we still need to convert `accumulator` back to the final dict `dstzset->dict`. However, we can directly use `dstzset->dict` to avoid the additional copy operation. This PR removes the `accumulator` dict and directly uses` dstzset->dict `to store the member-score mappings. - **Test** First, I added 300 unique elements to two sorted sets called 'zunion_test1' and 'zunion_test2'. Then, I tested `zunion` and `zunionstore` on these two sorted sets. The test results shown below indicate that the performance of both zunion and zunionstore improved about 31%. ### ZUNION #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 2713.41 requests per second latency summary (msec): avg min p50 p95 p99 max 146.252 3.464 153.343 182.015 184.959 192.895 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunion 2 zunion_test1 zunion_test2 Summary: throughput summary: 3543.84 requests per second latency summary (msec): avg min p50 p95 p99 max 108.259 2.984 114.239 141.695 145.151 160.255 ``` ### ZUNIONSTORE #### unstable ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 3168.07 requests per second latency summary (msec): avg min p50 p95 p99 max 157.511 3.368 183.167 189.311 193.535 231.679 ``` #### This PR ``` ./valkey-benchmark -P 10 -n 100000 zunionstore out 2 zunion_test1 zunion_test2 Summary: throughput summary: 4144.73 requests per second latency summary (msec): avg min p50 p95 p99 max 120.374 2.648 141.823 149.119 153.855 183.167 ``` --------- Signed-off-by: RayCao <zisong.cw@alibaba-inc.com> Signed-off-by: zisong.cw <zisong.cw@alibaba-inc.com>
moticless
added a commit
to redis/redis
that referenced
this pull request
Sep 25, 2024
…3566) This PR is based on valkey-io/valkey#829 Previously, ZUNION and ZUNIONSTORE commands used a temporary accumulator dict and at the end copied it as-is to dstzset->dict. This PR removes accumulator and directly stores into dstzset->dict, eliminating the extra copy. Co-authored-by: Rayacoo zisong.cw@alibaba-inc.com
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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.
In the past implementation of
ZUNION
andZUNIONSTORE
commands, we first create a temporary dict calledaccumulator
. After adding all member-score mappings toaccumulator
, we still need to convertaccumulator
back to the final dictdstzset->dict
. However, we can directly usedstzset->dict
to avoid the additional copy operation.This PR removes the
accumulator
dict and directly usesdstzset->dict
to store the member-score mappings.First, I added 300 unique elements to two sorted sets called 'zunion_test1' and 'zunion_test2'. Then, I tested
zunion
andzunionstore
on these two sorted sets. The test results shown below indicate that the performance of both zunion and zunionstore improved about 31%.ZUNION
unstable
This PR
ZUNIONSTORE
unstable
This PR