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

Improve the performance of obtaining write connections through double-check locks. #3229

Open
Chenrujie-85 opened this issue Mar 25, 2025 · 6 comments · May be fixed by #3228
Open

Improve the performance of obtaining write connections through double-check locks. #3229

Chenrujie-85 opened this issue Mar 25, 2025 · 6 comments · May be fixed by #3228

Comments

@Chenrujie-85
Copy link

Feature Request

Is your feature request related to a problem? Please describe

In the production environment, when the number of access requests sent by Lettuce as the Redis client reaches tens of thousands of qps, the latency of many access requests is long.

Describe the solution you'd like

When the getWriteConnection command is used to obtain a write connection in the PooledClusterConnectionProvider class, the system locks the connection and then checks whether the value of weiters[slot] is empty. When most elements in writers are assigned values, the performance is affected.

Describe alternatives you've considered

The code in getWriteConnection is changed to double-check lock. The system checks whether weiters[slot] is empty. If yes, the system locks it. Then, the system checks whether weiters[slot] is empty. If yes, the system checks whether weiters[slot] is empty. initializes weiters[slot]. Otherwise, returns weiters[slot].

Teachability, Documentation, Adoption, Migration Strategy

@tishun
Copy link
Collaborator

tishun commented Mar 26, 2025

Greetings @Chenrujie-85 ,

Before we delve deeper in your specific request could you please let me know what is the reason to choose to use a connection pool in your use case?

@tishun tishun added the status: waiting-for-feedback We need additional information before we can continue label Mar 26, 2025
@Chenrujie-85
Copy link
Author

Chenrujie-85 commented Mar 26, 2025

Greetings @Chenrujie-85 ,

Before we delve deeper in your specific request could you please let me know what is the reason to choose to use a connection pool in your use case?

Hello, I am using a single connection.
During my testing process, when the concurrency per minute was 3W, the average latency using the get method was around 1ms, with 60% of requests having latency below 1ms and 30% of requests having latency between 1ms and 5ms. When I used a double check lock and the concurrency per minute was 3W, the average latency using the get method was around 0.3ms, with 90% of requests having latency below 1ms

@tishun
Copy link
Collaborator

tishun commented Mar 27, 2025

My bad, I misunderstood your description.

You are addressing this piece of code:

               stateLock.lock();
                try {
                    if (writers[slot] == null) {
                        writers[slot] = CompletableFuture.completedFuture(connection);
                    }
                } finally {
                    stateLock.unlock();
                }

Correct? If so - yes, we could improve the code with a double checked lock.
Did you consider contributing this? Especially if you already have the setup to test it?

@Chenrujie-85
Copy link
Author

Chenrujie-85 commented Mar 27, 2025

My bad, I misunderstood your description.

You are addressing this piece of code:

           stateLock.lock();
            try {
                if (writers[slot] == null) {
                    writers[slot] = CompletableFuture.completedFuture(connection);
                }
            } finally {
                stateLock.unlock();
            }

Correct? If so - yes, we could improve the code with a double checked lock. Did you consider contributing this? Especially if you already have the setup to test it?

Yes, I'm working on this code, I've done the modification locally, and I've done the tests, and the results show that double-checking locks optimize performance.
If you agree, I will make a pull request.

@tishun
Copy link
Collaborator

tishun commented Mar 27, 2025

Oh, I see you already submitted a PR #3228

@tishun tishun added status: waiting-for-triage and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 27, 2025
@Chenrujie-85
Copy link
Author

Oh, I see you already submitted a PR #3228

Hello, I formatted the file and resubmitted it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants