Skip to content
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

Fix performance degradation due to non-caching of command info in Redis 6 Cluster #1355

Closed
wants to merge 2 commits into from

Conversation

yuuki
Copy link
Contributor

@yuuki yuuki commented Jun 5, 2020

What is the problem?

I observed performance degradation when using redis.ClusterClient in Redis 6.
pprof reported the following bottleneck caused by mutex lock (https://github.com/go-redis/redis/blob/a999d1ecd81303df8128547746c62358f78048f5/command.go#L2153-L2154).

ScreenShot 2020-06-04 at 9 01 38 PM

This mutex lock is for the client to cache COMMAND reply just once (https://github.com/go-redis/redis/blob/a999d1ecd81303df8128547746c62358f78048f5/cluster.go#L688).
However, I noticed that during the debugging of this performance degradation problem, for some reason the client executed COMMAND command every time.

What caused the problem?

I got the following error that has already reported in #1329 in Redis 6.

redis: got 7 elements in COMMAND reply, wanted 6

This error prevented the internal.once from setting the done flag
(https://github.com/go-redis/redis/blob/a999d1ecd81303df8128547746c62358f78048f5/internal/once.go#L54-L57). As a result, the client did not cache COMMAND reply.

This error is caused by the change of additional command categories in Redis 6.
I show the example of the output of COMMAND and COMMAND INFO reply.

  • Redis 5.0.8 (6 elements)
127.0.0.1:6379> COMMAND INFO xadd
1) 1) "xadd"
   2) (integer) -5
   3) 1) write
      2) denyoom
      3) random
      4) fast
   4) (integer) 1
   5) (integer) 1
   6) (integer) 1
  • Redis 6.0.1 (7 elements)
127.0.0.1:6379> COMMAND INFO xadd
1) 1) "xadd"
   2) (integer) -5
   3) 1) write
      2) denyoom
      3) random
      4) fast
   4) (integer) 1
   5) (integer) 1
   6) (integer) 1
   7) 1) @write
      2) @stream
      3) @fast

Thus, I have fixed go-redis to handle this 7th elements for additional ACL categories. Finally, I solved my problem of performance degradation.

@vmihailenco vmihailenco mentioned this pull request Jun 5, 2020
@yuuki yuuki force-pushed the support-commandsinfo-acl-flags branch from 949ccae to e328a73 Compare June 5, 2020 06:43
@vmihailenco
Copy link
Collaborator

Thanks for the PR - I've merged this a part of #1357

@yuuki
Copy link
Contributor Author

yuuki commented Jun 5, 2020

Thanks for your quick response!

sgswtky added a commit to sgswtky/redis that referenced this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants