Skip to content

Optimization: Avoid deferred array reply on ZRANGE commands BYRANK #10337

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 5 commits into from
Feb 24, 2022
Merged

Optimization: Avoid deferred array reply on ZRANGE commands BYRANK #10337

oranagra merged 5 commits into from
Feb 24, 2022

Conversation

@filipecosta90
Copy link
Contributor

@filipecosta90 filipecosta90 commented Feb 23, 2022 •
edited
Loading

Addresses #10310.

In progress:

  • re-measure now with the latest changes of unstable and assess if unstable vs 5.0 has the same performance.
  • reprofile pipeline 16 use-case
    • All details were added to issue comment [BUG] ZREVRANGE 50% slower after upgrading from 5.0.7 to 6.2.6 #10310 (comment).
    • pipeline 1 regression vs v5.0.7: this PR change will get us to approximately the same performance as v5 ( -3.30% ).
    • pipeline 16 regression vs 5.0.7: on unstable we're -33.15% against v5.0. With this PR we're -15% which means there is still a gap on the performance vs v5. In a nutshell we've added some logic/features that cost ~=8% CPU cycles. So we can explain the majority of the difference. @oranagra please advise if there is something we can improve/reduce the overhead on those 8% CPU cycles extra.
recipe:

rm dump.rdb ; src/redis-server --save "" &
redis-cli zadd zz 0 a 1 b 2 c 3 d 4 e 5 f
memtier_benchmark --pipeline 16 --command "zrange zz 0 -1" --hide-histogram
pipeline 5.0.7 ops/sec unstable ( c81c7f5 ) % change vs v5.0.7 this PR ( d95ae93 ) % change vs v5.0.7 this PR after reply to replyProto ( 9478d53 ) % change vs v5.0.7
1 179077 143278 -19.99% 173164 -3.30% 172104 -3.89%
16 1022965 683870 -33.15% 851282 -16.78% 866938 -15.25%

@oranagra oranagra linked an issue Feb 24, 2022 that may be closed by this pull request
[BUG] ZREVRANGE 50% slower after upgrading from 5.0.7 to 6.2.6 #10310
Closed
@filipecosta90
Revert "Avoid calling updateCachedTime() --> ustime() on call() given…
6c0dada 
… we already have computed a timestamp just before"

This reverts commit 57b2593.
@filipecosta90 filipecosta90 changed the title [In-Progress] Avoid deferred array reply on genericZrangebyrankCommand() when consumer type is client Avoid deferred array reply on genericZrangebyrankCommand() when consumer type is client Feb 24, 2022
@filipecosta90 filipecosta90 mentioned this pull request Feb 24, 2022
Closed
@filipecosta90 filipecosta90 added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Feb 24, 2022
@oranagra
Copy link
Member

oranagra commented Feb 24, 2022

@filipecosta90 does the unstable above contains #9934 and #10334, or is it before them?

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Feb 24, 2022

@filipecosta90 does the unstable above contains #9934 and #10334, or is it before them?

it contains it. I've used c81c7f5

oranagra
oranagra reviewed Feb 24, 2022
View reviewed changes
@oranagra oranagra changed the title Avoid deferred array reply on genericZrangebyrankCommand() when consumer type is client Optimization: Avoid deferred array reply on ZRANGE commands BYRANK Feb 24, 2022
@oranagra oranagra merged commit 1dc89e2 into redis:unstable Feb 24, 2022
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 24, 2022
@oranagra oranagra mentioned this pull request Feb 28, 2022
Merged
This was referenced Apr 27, 2022
Closed
Merged
oranagra added a commit that referenced this pull request Apr 27, 2022
@filipecosta90 @oranagra
Optimization: Avoid deferred array reply on ZRANGE commands BYRANK (#…
ef78757 
…10337)

Avoid deferred array reply on genericZrangebyrankCommand() when consumer type is client.
I.e. any ZRANGE / ZREVRNGE (when tank is used).
This was a performance regression introduced in #7844 (v 6.2) mainly affecting pipelined workloads.

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 1dc89e2)
@mgravell mgravell mentioned this pull request Aug 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

Assignees

No one assigned

Labels

action:run-benchmark Triggers the benchmark suite for this Pull Request release-notes indication that this issue needs to be mentioned in the release notes

Projects

Redis 6.2 Backport
Status: Done
Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] ZREVRANGE 50% slower after upgrading from 5.0.7 to 6.2.6

2 participants

@filipecosta90 @oranagra