Conversation
Remove `AbstractPoolObject` inheritance from Stats Use `ConcurrentQueueWithPool` instead of `ConcurrentBoundedQueue`
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
Looks good to me, just a couple of nits and a question.
I have a small difference of opinion with regard to ConcurrentQueueWithPool, not because the code therein doesn't make sense, it makes total sense, but because I think we have a class there that is doing two different things: pool and queue. We've already spoken about how these might make sense to be used together in a few scenarios, but I also feel it's initially a little misleading at a high-level (of course also something we could address with good docs). I definitely don't think we should change it here, just a thought of maybe something we might want to refactor in the future.
Nice stuff.
| Assert.True(queue.TryDequeueFromPool(out var v)); | ||
| Assert.AreEqual(42, v); | ||
|
|
||
| Assert.False(queue.TryDequeueFromPool(out var _)); |
There was a problem hiding this comment.
nit: a small comment here to explain how this call will return false and timeout because the task has not yet put 43 into the pool'd queue would be nice.
There was a problem hiding this comment.
I have added a more detailed comment in the code.
|
@truthbk : I have added an implementation note for |
3 participants
This PR adds several performance improvements:
MetricSerializer: Improve double serialization by ~15%.BufferBuilder.Addby ~30%MetricSerializerby 110%The overall improvement is ~ 70%.
Reviewing commit by commit is recommended.
This PR must be merged after #163.