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

Fallback MGetCache to MGet when DisableCache=true #567

Closed
jo-me opened this issue Jun 14, 2024 · 8 comments
Closed

Fallback MGetCache to MGet when DisableCache=true #567

jo-me opened this issue Jun 14, 2024 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jo-me
Copy link

jo-me commented Jun 14, 2024

Hi,

I'm playing around with rueidis as drop-in replacement for redis-go, but I'm seeing some strange performance in a (local) comparison.
On my dev machine I have redis running in docker and a simple API service that checks redis based on the IDs given in the API requests.
All tests were done with Rueidis v1.0.38 and go-redis v9.5.3 using go 1.22.3.

For my simple test I'm setting 5 values in redis with~2KB each and hammer my API with k6 to see how many requests per second I get when I request all 5 elements over and over.

Without any caching involved, I have around 50 rps (have to recreate the 5 elements).
With go-redis I have around 25000 rps

Now, when switching to ruedis I tried out 3 different configurations:
MGetCache with client option DisableCache:true: ~500 rps
MGetCache with client option DisableCache:false: ~50000rps
DoMultiStream: ~30000 rps

So, with client-side caching the rps are considerably higher as expected (although the difference is not that much probably due to redis being on the same host), but with disabled client-side cache, there seems to be something strange going on.
Aside from the DisableCache option the code is exactly the same.

To my understanding Mget and Mgetcache (without client-side caching) should behave identically and I would have expected a similar result as with go-redis.

Any idea what could be the issue?

Thanks!

Jochen Mehlhorn [email protected], Mercedes-Benz Tech Innovation GmbH

Provider Information

@rueian
Copy link
Collaborator

rueian commented Jun 14, 2024

Hi @jo-me,

Thank you for sharing your test results. Unlike MGet helper, MGetCache with DisableCache=true currently sends each key separately to Redis.

Could you also test MGet? I think it would be similar to go-redis. If that is the case, we can fallback MGetCache to MGet when DisableCache=true in the future.

@jo-me
Copy link
Author

jo-me commented Jun 17, 2024

Hmm,
I repeated the test this morning with the same setup as described above but another two variants for the no caching code path:

client.Do(ctx, c.client.B().Mget().Key(keys...).Build()).AsStrSlice()
and
rueidis.MGet(client, ctx, keys)

but both were on par with the MgetCache variant with DisableCache=true
I also tried it without the DisableCache client option in case it somehow influences the Do/Mget commands, but it did non make any difference.

My client initialization is like this btw:

	client, err := rueidis.NewClient(rueidis.ClientOption{
		InitAddress:   []string{fmt.Sprintf("%s:%d", config.Address, config.Port)},
		MaxFlushDelay: 500 * time.Millisecond,
		DisableCache:  config.DisableClientSideCaching,
	})

@jo-me
Copy link
Author

jo-me commented Jun 17, 2024

When I look at redis monitor I cannot confirm what you're saying.
MGetCache calls with both DisableCache true and false result in MGET requests getting sent to redis.
I do not see single GET requests - only for SET commands (even though I'd have expected to be the pipelining to kick in and see a MSET). By the way, I'm using the standard redis docker image with no extra configuration.

One difference I see in redis monitor though is the connection:

go-redis:
1718607777.106890 [0 172.17.0.1:40738] "hello" "3"
1718607777.107103 [0 172.17.0.1:40738] "client" "setinfo" "LIB-NAME" "go-redis(,go1.22.3)"
1718607777.107106 [0 172.17.0.1:40738] "client" "setinfo" "LIB-VER" "9.5.3"

rueidis:
1718608372.964534 [0 172.17.0.1:44636] "HELLO" "3"
1718608372.964593 [0 172.17.0.1:44636] "CLIENT" "TRACKING" "ON" "OPTIN"
1718608372.964600 [0 172.17.0.1:44636] "CLIENT" "SETINFO" "LIB-NAME" "rueidis"
1718608372.964602 [0 172.17.0.1:44636] "CLIENT" "SETINFO" "LIB-VER" "1.0.38"
1718608372.966053 [0 172.17.0.1:44636] "CLUSTER" "SHARDS"
1718608372.967840 [0 172.17.0.1:44652] "HELLO" "3"
1718608372.967853 [0 172.17.0.1:44652] "CLIENT" "TRACKING" "ON" "OPTIN"
1718608372.967855 [0 172.17.0.1:44652] "CLIENT" "SETINFO" "LIB-NAME" "rueidis"
1718608372.967865 [0 172.17.0.1:44652] "CLIENT" "SETINFO" "LIB-VER" "1.0.38"
1718608372.968207 [0 172.17.0.1:44652] "CLUSTER" "SHARDS"

maybe that is causing the unexpected behavior?

@jo-me
Copy link
Author

jo-me commented Jun 17, 2024

Ohhhh, I think I know where the behavior is coming from.

When I remove the MaxFlushDelay from the client config (or set ForceSingleClient to true) I see the single GET requests that MGetCache is doing when DisableCache=true.

Removing MaxFlushDelay also "fixed" the performance problem that I was seeing. It is now comparable to go-redis.
I was under the impression that MaxFlushDelay would only affect SET requests when it probably affects all writes to the connection (not only writes to redis).

Anyway, falling back to MGET in the DisableCache=true mode would probably be a good idea as it is what you'd expect as a user and is also faster.

What abound when clientside caching is enabled though? Shouldnt all keys missing from the local cache be requested at once?
What i see in the latter case is the following which (to my understanding) executes a transaction for each GET).

1718615983.092405 [0 172.17.0.1:40372] "CLIENT" "CACHING" "YES"
1718615983.092432 [0 172.17.0.1:40372] "MULTI"
1718615983.092442 [0 172.17.0.1:40372] "PTTL" "545554684"
1718615983.092451 [0 172.17.0.1:40372] "GET" "545554684"
1718615983.092454 [0 172.17.0.1:40372] "EXEC"
1718615983.092459 [0 172.17.0.1:40356] "CLIENT" "CACHING" "YES"
1718615983.092461 [0 172.17.0.1:40356] "MULTI"
1718615983.092463 [0 172.17.0.1:40356] "PTTL" "545554685"
1718615983.092465 [0 172.17.0.1:40356] "GET" "545554685"
1718615983.092467 [0 172.17.0.1:40356] "EXEC"
1718615983.092468 [0 172.17.0.1:40356] "CLIENT" "CACHING" "YES"
1718615983.092469 [0 172.17.0.1:40356] "MULTI"
1718615983.092471 [0 172.17.0.1:40356] "PTTL" "545554858"
1718615983.092473 [0 172.17.0.1:40356] "GET" "545554858"
1718615983.092474 [0 172.17.0.1:40356] "EXEC"
1718615983.092475 [0 172.17.0.1:40356] "CLIENT" "CACHING" "YES"
1718615983.092476 [0 172.17.0.1:40356] "MULTI"
1718615983.092478 [0 172.17.0.1:40356] "PTTL" "545554854"
1718615983.092479 [0 172.17.0.1:40356] "GET" "545554854"
1718615983.092481 [0 172.17.0.1:40356] "EXEC"
1718615983.092530 [0 172.17.0.1:40368] "CLIENT" "CACHING" "YES"
1718615983.092533 [0 172.17.0.1:40368] "MULTI"
1718615983.092535 [0 172.17.0.1:40368] "PTTL" "545554857"
1718615983.092538 [0 172.17.0.1:40368] "GET" "545554857"
1718615983.092539 [0 172.17.0.1:40368] "EXEC"

@rueian
Copy link
Collaborator

rueian commented Jun 17, 2024

When I remove the MaxFlushDelay from the client config (or set ForceSingleClient to true) I see the single GET requests that MGetCache is doing when DisableCache=true.

That's weird. MaxFlushDelay should have nothing to do with the MGetCache helper. ForceSingleClient as well, unless you were using Redis Cluster.

Removing MaxFlushDelay also "fixed" the performance problem that I was seeing.

500 milliseconds is indeed too large. I recommend 20 microseconds (#156 (comment)).

What abound when clientside caching is enabled though? Shouldnt all keys missing from the local cache be requested at once?
What i see in the latter case is the following which (to my understanding) executes a transaction for each GET).

Yes, that is an optimization we can do and that isn't easy to implement.

@rueian rueian added enhancement New feature or request good first issue Good for newcomers labels Jun 17, 2024
@rueian rueian changed the title MGetCache with DisableCache unexpectedly slow Fallback MGetCache to MGet when DisableCache=true Jun 17, 2024
@SoulPancake
Copy link
Contributor

@rueian Do we modify this in the helper? How can we check the clientOption DisableCache is set to true in that scope?

@rueian
Copy link
Collaborator

rueian commented Jun 18, 2024

@rueian Do we modify this in the helper? How can we check the clientOption DisableCache is set to true in that scope?

For example:

case *singleClient, *sentinelClient:

@rueian
Copy link
Collaborator

rueian commented Jun 25, 2024

Resolved.

@rueian rueian closed this as completed Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants