-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Make SocketManager worker count configurable #1115
Conversation
To echo what I said in the linked issue - can I ask more info about the scenarios where this is manifesting? I just want to make sure we're solving the correct problem. Do you have some log/trace/etc that indicates that we're exhausting the dedicated pool? Also: what is the configuration here? How many multiplexers? How many connections? Is this cluster? etc... I want to better understand the scenario in which this would be necessary. |
Sure. Our current scenario is a message processing system. We're trying to achieve a good balance for throughput/CPU usage. We process many small messages concurrently. Each message will incur ~5-10 Redis operations, all async. When trying to find the sweet spot for how many consumers to run concurrently with different settings these are our findings. Processed messages / second is the metric we care about. Was measured on our production setup, Azure P3 Redis, not clustered. Processing on a 4 CPU machine, dotnet core 2.2 console app using latest version of StackExchange.Redis. When testing we are the sole consumer of this Redis instance.
So for our use case it seems a single Multiplexer with a SocketManager that has a larger thread pool is a lot more performant. It's not TimeoutExceptions that's our main issue, even though we can get them when using a smaller SocketManager worker pool.
|
Perfect, in that case - seems legit. You've obviously done a lot of work here to understand context etc - but you'd be amazed how often we get requests to change things without that level of effort, just a random "this might fix it?". It surprises me that you've managed to saturate the pool without "cluster", though... a small part of me is wondering "what are they doing?", but... we may never know! In terms of review feedback - since we already have a public constructor that takes public SocketManager(string name) // <== remove optional param here
: this(name, DEFAULT_WORKERS, false) {}
public SocketManager(string name, bool useHighPrioritySocketThreads)
: this(name, DEFAULT_WORKERS, useHighPrioritySocketThreads) {}
public SocketManager(string name = null, int workerCount = 0, // this *was* the private .ctor
bool useHighPrioritySocketThreads = false)
{
if (workerCount <= 0) workerCount = DEFAULT_WORKERS;
// ... rest of the "real" code here
} reasons:
I'm happy to merge and make those changes myself, or you're welcome to make them as part of the PR. Just let me know your preference. |
That makes sense, i can update the PR. Would you prefer replacing the private CTOR or calling it? public SocketManager(string name = null, int workerCount = 0, bool useHighPrioritySocketThreads = false)
: this(name, useHighPrioritySocketThreads, workerCount) { }
private SocketManager(string name, bool useHighPrioritySocketThreads, int workerCount)
{
if (workerCount <= 0) workerCount = DEFAULT_WORKERS; // <--this is new
// ... rest of the "real" code here
} |
might as well replace it, i.e. make it |
do you have any idea when this PR may merge? |
merging, thanks |
@mgravell any plans when 2.1.0 will be released with these changes? |
I have another PR I want to review and dogfood, as it could significant help on cluster, then we should be good for a deploy |
With async-heavy workloads the default Shared instances' 10 workers are not always enough. With a dedicated SockerManager the worker count is only 5 and not configurable.
This PR
Should solve #1035