Skip to content

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

Merged
oranagra merged 5 commits intoredis:unstablefrom
filipecosta90:zrange.patch
Feb 24, 2022
Merged

Optimization: Avoid deferred array reply on ZRANGE commands BYRANK#10337
oranagra merged 5 commits intoredis:unstablefrom
filipecosta90:zrange.patch

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 <[email protected]>
(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