[Proposal] Pool WriteContext per thread #469
Conversation
Without waiting for next libuv pass Fix for potential regression in aspnet#363 due to bug fix.
- This allows all connections accepted by the same thread to share a pool
benaadams
force-pushed
the
static-writepool
branch
from
c19a480 to
f760c23
Compare
benaadams
force-pushed
the
static-writepool
branch
from
8af4326 to
1edc646
Compare
benaadams
force-pushed
the
static-writepool
branch
from
1edc646 to
6e7186b
Compare
|
|
||
| private static WaitCallback _returnBlocks = (state) => ReturnBlocks((MemoryPoolBlock2)state); | ||
|
|
||
| [ThreadStatic] |
There was a problem hiding this comment.
I'm afraid of adding a ThreadStatic. We don't use ThreadStatics anywhere else in Kestrel that I'm aware of.
I just checked the default maximum number of worker threads on my machine using ThreadPool.GetMaxThreads, and it was 32,767. I don't like having potentially that many static queues. Considering that each queue could pool up to 128 WriteContexts (and you just calculated the size WriteContext being 88 bytes), that's up to ~370MB of WriteContexts that will never be freed. That's not even considering the size of the queues themselves.
There was a problem hiding this comment.
Don't hold off merging #466 based on this (no issues in that one; and this is straight on top)
Know what you mean about ThreadStatics though been doing some investigation.
So the ThreadStatic is GC'd after thread exit; assuming it has no other outstanding strong references - which should be ok for this. Objects with a finalizer aren't so good as they'll hang around till the finalizer runs - but the WriteContext that's been reset is just a POCO managed type; so the queue will be happily GC'd when the threadpool scales back down.
If you are up to 32k of live threadpool threads; then the 32GB of thread stack space they need might be more of an issue than the 370MB of pooled WriteContexts.
However... I'm not sure this PR is a great win; it doesn't reduce any contention, as there already isn't any; but it should reduce the memory for a very large number of connections - but that scenerio will probably have all sorts of other stuff to look at.
Happy to close for now and come back to; if it is an issue?
|
There's nothing in here that I need if I just want to merge #466, right? |
benaadams
changed the title
|
Closing this; can revisit later |
Continuation of SocketOutput Allocation Reductions #466
With extra of "Pool WriteContext per thread rather than connection"
Resolves #165 Pool UvWriteReq objects
Resolves #289 High Allocator DoWriteIfNeeded
Resolves #290 High Allocator SocketOutput.Write