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

recommend enableAutoPipelining? #257

Open
2 tasks done
Uzlopak opened this issue Aug 15, 2022 · 13 comments
Open
2 tasks done

recommend enableAutoPipelining? #257

Uzlopak opened this issue Aug 15, 2022 · 13 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 15, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

I wonder if we get some performance benefit if we recommend enableAutoPipelining?

@mcollina
Copy link
Member

Likely so

@Eomm Eomm added the good first issue Good for newcomers label Sep 3, 2022
@dancastillo
Copy link
Contributor

would this just be a documentation update? Seeing as option is available in ioredis https://github.com/luin/ioredis/blob/main/lib/redis/RedisOptions.ts#L136

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 8, 2022

I dont think this is a documentation only task.

I assume that for really using autoPipelining, we have to replace the manually set pipelines with a lua script. Something like
https://github.com/wyattjoh/rate-limit-redis/blob/main/src/lib.ts#L58

Then I think we can make use of autoPipelining.

@mcollina
Copy link
Member

mcollina commented Dec 9, 2022

If we enable autopipelining we can just remove those two pipeline commands.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 9, 2022

Sure, but what if the implementator does not use ioredis but node-redis or something like that. In the readme.md we say, that we recommend ioredis, but other redis clients are also possible. And if they dont implement autopipelining then we have a broken solution.

@climba03003
Copy link
Member

climba03003 commented Dec 9, 2022

Auto-pipelining meaning they will defer some of the operation and send them in groups in unexpected way.
It also means that we are actually defering the request to be process since rate-limit should happen in the earliest stage.

I don't think how the impact was, maybe it will increase latency but result in higher throughput.
The change of default should be carefully considered and MUST be benchmark.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 9, 2022

Probably using lua improves the perforrmance. It would also mean that we dont need to pipeline in our redis store. It is then optional to use auto pipelining or not.

@mcollina
Copy link
Member

Auto-pipelining meaning they will defer some of the operation and send them in groups.

This is incorrect. In fact, it will execute ops sooner because they will be batched with previous ops.

In my experience using Lua does not improve things in similar cases. The cost here is driven by head-of-line blocking.

@climba03003
Copy link
Member

This is incorrect. In fact, it will execute ops sooner because they will be batched with previous ops.

I am not sure about this statement, when I look through the code of ioredies it should be delaying the network connection in order to stack batch of command.

https://github.com/luin/ioredis/blob/e41c3dc880906e8aad73332837bf233f65d12e67/lib/autoPipelining.ts#L144-L175

When it do not use autoPipelining, the connection is not limit to 1. It actually send concurrently.

autoPipelining seems to increase throughput only because it reduce numbers TCP handshake.

@mcollina
Copy link
Member

mcollina commented Dec 24, 2022

I invented that tecnique, watch the video: https://m.youtube.com/watch?v=0L0ER4pZbX4.

Only one command could be in flight on a socket connected to Redis. Without autopipelining, all commands are queued and run in order. With autopipelining:

  1. If the socket has not any commands in flight, the command is deferred by 1 macrotick (wait for other command in the same event loop run).
  2. if the socket has a command in flight, all subsequent commands are batched and sent as single pipeline.

This mitigate head of line blocking in Redis. Considering 100 commands and 10ms RTT this will give a save of 990ms waiting time.
Pretty good.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 24, 2022

I guess we can check if the redis option is of ioredis instance and if so check if autoPipeline is enabled. I so use without pipeline() if autoPipelin is true.

@gurgunday
Copy link
Member

gurgunday commented Oct 22, 2023

node-redis seems to have built-in autoPipelining, which could help users not think about these kinds of things

https://github.com/redis/node-redis#auto-pipelining

I'm not sure what we should do here, I honestly don't know which one is better, or if that's a wrong way to look at it, which one is Redis Ltd's preferred/main package for node

@gurgunday gurgunday added help wanted Extra attention is needed and removed good first issue Good for newcomers labels Oct 22, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 24, 2023

@gurgunday

The problem is, that we cant use autoPipelining, because our redis store is creating a pipeline for each call. So autoPipelining wont pipeline that again.

I think we have to create a custom command in redis, which does the incr and pttl call, so we dont use pipeline anymore and autoPipeline takes effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants