-
Notifications
You must be signed in to change notification settings - Fork 346
Simplify BufferPool #67
Comments
Our analysis of ObjectPool in Roslyn has shown that the most common scenario is a burst of ferocious borrow/return activity from the same, but random thread. That could be explained by the fact that the most benefit from pooling comes when objects are used for very short duration. Objects that are taken from the pool for long times or permanently do not benefit as much from pooling. So, as a result of this, various threads transition into mode of heavy pool use in and out, but rarely at the same time. Not sure if that translates to the scenarios for BufferPool. I think it might be likely. Because of the above, in Roslyn we can assume (and see in profiles), that
I had some limited experiments with striped pool - ie have 4 sub-pools and pick one based on the current threadID to reduce chances of sharing. The additional complication did not pay for itself since we did not have enough contention in the first place. Another observation is that there are lots of scenarios (like scratch buffer or a builder) when a task needs briefly just one object at a time, so many requests could be satisfied with just one element of the cache. Perhaps this info would be useful. |
Our object pool is basically a Get/Free API wrapped around a single object + a fixed size array. |
Regarding ManagedBufferPool specifically, http archive.org interesting stats is commented as the justification for the 2048000 bytes max buffer size default in System/Buffers/ManagedBufferPool.cs. |
@sokket, I think this issue is resolved. If so, please close it. But please do continue the discussion about the max size of pooled buffers. It's important that we get it right. Having said that, I looked at the data and it seems like the total size of all assets is approaching 2MB per page. But I am not sure this is relevant to this discussion. Separate assets will be sent/received using separate buffers, correct? |
I agree, I think this issue can be closed. @codespare if you think the default buffer size is incorrect, please open an issue and tag me so we can have a discussion about it there and have an action item to tag any PR with to more easily track why the change would occur :) |
The pool is quite complex and I don't think the complexity is worth it. Interlocked operations are very expensive, and I think performance sensitive scenarios should use a pool per thread. Also, the interlocked synchronization is not much faster than locks but it causes some buffers to be dropped on the heap.
The text was updated successfully, but these errors were encountered: