redis
Multi node pipeline executor
This allow clients to provide an ExecutorService at pipeline creation instead of having a new ExecutorService created at every pipeline Sync call. An Executor service creation will create new threads, which are expensive resource to creates. In a high throughput application developed internally, we are writing at a rate of ~100k set per seconds, on a six node cluster, on an instance equivalent to an EC2 m5.12xlarge. The creation of threads uses 40% of CPU, and adds substantial latency. This new approach will allow clients to send a pooled Executor that is tuned to there load patterns.
This change is non breaking, but will come with a slight optimization for the clients currently using the created thread pool. In the current approach even if a pipeline has a single connection to close the Executor service will create MULTI_NODE_PIPELINE_SYNC_WORKERS threads. In the default mode would mean wasting 2 thread creation.
From: https://stackoverflow.com/questions/5483047/why-is-creating-a-thread-said-to-be-expensive
Thread lifecycle overhead. Thread creation and teardown are not free. The actual overhead varies across platforms, but thread creation takes time, introducing latency into request processing, and requires some processing activity by the JVM and OS. If requests are frequent and lightweight, as in most server applications, creating a new thread for each request can consume significant computing resources.
From Java Concurrency in Practice By Brian Goetz, Tim Peierls, Joshua Bloch, Joseph Bowbeer, David Holmes, Doug Lea Print ISBN-10: 0-321-34960-1
Java thread creation is expensive because there is a fair bit of work involved:
- A large block of memory has to be allocated and initialized for the thread stack.
- System calls need to be made to create / register the native thread with the host OS.
- Descriptors need to be created, initialized and added to JVM-internal data structures.
@dolavb Thank you for your effort to improve Jedis. Your concern is justified. But we have our hands full ATM. We'll try to get to this PR ASAP.
Executed some more load testing, and we have host spending 60% our total CPU usage on starting threads.
This shows the latency resulting from starting thread, 5 times the time than waiting on Redis to finish the write.
This shows the CPU usage resulting from the thread starting.
lets start considering on a pool of executorService instances. And discuss tradeoffs of each solution we have. For sure we are doing this for performance but we need to regard other aspects from API standpoint, like principles of information hiding and encapsulation .
@atakavci Yes, I think the API considerations are important. It is unfortunate that this now have an API that is essentially a global variable in the static field. Ohterwise, I would have chosen one of the typical approaches:
- Have an optional ExecutorService in the client config.
- Handle the option at the implementation level, if present parallel calls, if absent, serial calls.
- Have a thread pool configuration in the client config (threadcount and the such).
- And expose the resulting threadpool in some way for clients who are adding JMX sensor or other things to their threadpools, like in my usecase.
These approaches would keep changes to the API small by avoiding injecting the ExecutorService in the client's pipeline method. The executorService would be a field in the client config that would be injected from there when creating a pipeline.
I did not go for any of these approach because of the unfortunate public static variable. There might be a way to do it, but I am not sure it would be simpler if we want to maintain backward compatibility. So do we want to break people who have started depending on that static field?
As for information hiding, I am not sure I understand, a ThreadPool is not information it is resources, if it is the responsibility of the client to create it, it needs to be exposed in some way. I am running a system where resources are important, we monitor all of our threadpools for tuning, and we do this using JMX sensors. I would not use a lib that hides a threadpool.
hey @dolavb i partially agree on what you stated. There are couple of ways to do it without introducing a breaking change.
i was thinking suggestion to "have a pool of ExecutorService " is kind of self descriptive but let me come up with a draft or at least some pseudo code around it.
BTW, with information hiding i m basicaly referring to exposing the use of a ExecutorService is not an ideal thing to do. The ones using public API has nothing to do with how(or with what) it actually runs under the hood, and it should remain that way. Leaving a small question here to clarify; what happens as library maintainers we decide to move away from existing ExecutorService implementations tomorrow ?
@atakavci I agree there are a couple of ways to do it without adding a breaking change, but I believe the complexity is not worth it. But your call.
@dolavb , @ggivo please take a look at this and let me know what you think of.
downside of this, as is, the lack of mechanism to collect the servicePool
@dolavb @atakavci
downside of this, as is, the lack of mechanism to collect the
servicePool
The downside mentioned is a critical one since there is no way to free the resources at will Also, with this approach, we still have the configuration of the pool/executors hidden to the user, and they lack the option to optimise it for their use case.
Providing the option for using external ExecutorService will allow for customisable ExecutorService based on the user's needs, and users will have control over the lifecycle of the ExecutorService.
Why do we consider this to be a breaking change if we preserve the default behaviour to be the existing one?
- If an external
ExecutorServiceis not explicitly provided, we still use the existing approach (create newExecutorServiceusing the statically provided configuration ) - To use shared
ExecutorService, users should explicitly set it (viaJedisClientConfig, or provide it when constructing the pipeline)
From API perspective, we are adding a new configuration option, which should not be a breaking change
to get it clear, no breaking solutions proposed in this PR as i see.
yet i am on behalf of confirming if we are comfortable at adding Executors to public API?
Its nice that it gives great control to the app!
May be we should emphasize/declare more on two modes of execution with MultiNodePipelineExecutor, not sure.
Question: Does it make sense to introduce a simple interface that provides only submit(...) on it ?
Having shared ExecutorService and owned one interchangeable like the way here, could it lead to potential confusion or misuse in the future ?
Maybe we should emphasize/declare more on two modes of execution with
MultiNodePipelineExecutor, not sure. Question: Does it make sense to introduce a simple interface that provides onlysubmit(...)on it ? Having shared ExecutorService and owned one interchangeable like the way here, could it lead to potential confusion or misuse in the future?
What I can suggest here is to document the API with a clear statement that when external ExecutorService is provided it's lifecycle is not managed by the Jedis client and is the user's responsibility for proper resource management (shutdown).
In addition, I suggest wrapping the ExecutorService in simple PipelineExecutor interface with submit/shutdown methods, which will allow us some control in the future if we need to add logging and metrics or even implement an executor that performs the call in the caller thread directly.
public interface PipelineExecutor {
void submit(Runnable task);
default void shutdown() {
// you would implement shutdown behavior
}
static PipelineExecutor from(ExecutorService executorService) {
return new PipelineExecutor() {
@Override
public void submit(Runnable task) {
executorService.submit(task);
}
@Override
public void shutdown() {
executorService.shutdown(); // Shuts down the ExecutorService
}
};
}
}
@dolavb @atakavci Appreciate the time spent & effort in this issue. Let me know if the above suggestion makes sense
@ggivo I agree this is a better approach.
@atakavci upon review of your gist I have the following question, why the countdownlatch? Since the introduction of CompletableFuture (JDK8), future composition is the idiomatic way to wait/merge threads. I don't see any backward compatibility concern here since JDK7 is EOL.
i dont mind if its countdownlatch or Futures, i just did it in min lines of change with existing code. rest is the diff editor's trick.
While working on this I saw this. Do I understand correctly that we shutdown the topologyRefreshExecutor in the JedisClusterInfoCache whenever we close a pipeline?
I apologize for the lack of clean reference. I am not the most versed with GitHub, and could not find a way to better reference this here.
While working on this I saw this. Do I understand correctly that we shut down the topologyRefreshExecutor in the JedisClusterInfoCache whenever we close a pipeline?
I don't think that is the case, but let's keep the discussion in referred PR, not to clutter this one
As part of this PR, should we mark public static volatile int MULTI_NODE_PIPELINE_SYNC_WORKERS = 3; as deprecated? With a deprecation comment like this:
* @deprecated Client using this approach are paying the thread creation cost for every pipeline sync. Clients
* should use refer to {@link JedisClientConfig#getPipelineExecutorProvider()} to provide a single Executor for
* gain in performance.
I would advise for having a default Executor to be provided in the future and this would be in preparation of that change. Otherwise I will keep the comment, but remove the deprecated annotation.
@ggivo This makes for a bigger change but offer a path away from the current static implementation. Let me know what you think.
@ggivo This makes for a bigger change but offer a path away from the current static implementation. Let me know what you think.
@dolavb Sorry for the delay. Got distracted with other tasks, but I will take a look till the end of the week
As part of this PR, should we mark
public static volatile int MULTI_NODE_PIPELINE_SYNC_WORKERS = 3;as deprecated? With a deprecation comment like this:
I would advise for having a default Executor to be provided in the future and this would be in preparation of that change. Otherwise I will keep the comment, but remove the deprecated annotation.
@dolavb @atakavci We are dealing with three distinct issues, and I suggest addressing them separately:
1️⃣ Performance issues due to creating an executor per pipeline • The best approach here is to allow the use of an external executor in Pipelines, as initially suggested in this PR. • This solution enables users of the client library to manage the executor lifecycle and configuration optimally.
2️⃣ Sharing an executor across all Pipelines from the same Jedis client • This needs further discussion, as it introduces new challenges. • There is no universal default configuration that works for all users, given varying usage patterns. • A misbehaving Pipeline could impact others sharing the same executor. • I suggest opening a dedicated GitHub Issue to discuss the trade-offs and potential solutions.
3️⃣ Static configuration of the default pipeline executor • I agree that this should be configurable at the Jedis client level. • However, rather than replacing the current behavior with (2), we could introduce a ClientOption to allow overriding the static executor while maintaining backward compatibility. • Again, I recommend opening a separate issue for discussion.
Next Steps for This PR
I suggest we focus solely on 1️⃣ in this PR, as it directly addresses the performance issue and provides immediate value. For 2️⃣ and 3️⃣, we can track them separately via new issues/PRs to ensure a structured approach.
Thoughts? 🚀
Ultimately I don't mind. We have had that forked and rolled out and is working well with performance issues elsewhere in Redis.
On 1, I agree, if the previous approach is preferred, then I do not understand some of the comments in this PR.
On 2, I think this approach gives you just that, you can provide the executor you want to the client. I believe the cross pipeline concern is a non issue, executors offer enough implementations or offers you ways to implement what you need enough to cover all usage pattern. If you think of it in relation of your connection count, then it becomes simple to reason about. This is what we did by monitoring both thread and connection pool under load test.
On 3, I disagree, the static configuration as a global variable is quite a code smell and should be taken out to avoid anybody else assuming this is something accepted in this code base.
But ultimately I want the solution to the creation of executor services to be solved as it is not just bad for us, but for everyone using this, resulting in a wasteful CPU usage. I can revert back to what I had done before, even though this does I think this is not the ideal implementation. You tell me what to do, and will update the PR.
It's a good idea to provide a Executor/ExecutorService by user, instead of create a new one for each pipeline. It's can't used in high-concurrency/cost scenarios. the default implement shouldn't to create a new threadpool for each pipeline.
If the cluster has only 3 nodes, we can even run it directly in the work thread by
Executor executor = run -> run.run()
Ultimately I don't mind. We have had that forked and rolled out and is working well with performance issues elsewhere in Redis.
On 1, I agree, if the previous approach is preferred, then I do not understand some of the comments in this PR.
On 2, I think this approach gives you just that, you can provide the executor you want to the client. I believe the cross pipeline concern is a non issue, executors offer enough implementations or offers you ways to implement what you need enough to cover all usage pattern. If you think of it in relation of your connection count, then it becomes simple to reason about. This is what we did by monitoring both thread and connection pool under load test.
On 3, I disagree, the static configuration as a global variable is quite a code smell and should be taken out to avoid anybody else assuming this is something accepted in this code base.
But ultimately I want the solution to the creation of executor services to be solved as it is not just bad for us, but for everyone using this, resulting in a wasteful CPU usage. I can revert back to what I had done before, even though this does I think this is not the ideal implementation. You tell me what to do, and will update the PR.
The initial solution proposed has a small API surface change and solves the main issue, my thinking was if wit make sense to do the change in stages since the second solution requires a bigger change and might require more discussion till we get to final solution
Can you provide the timeout parameters? In my scenario, when the pipeline takes a long time, it is desirable to timeout and release the connection and thread early. @dolavb
Another suggestion, grouping the request params by node first, and then execute them separately, finally merge them. So that connection can be used efficiently, not wait for each other.