Use entry.unguardedRecycle() instead of entry.recycle() in method MemoryRegionCache.freeEntry(...) #13727
Conversation
…oryRegionCache.freeEntry(...)
| @@ -438,7 +438,7 @@ private void freeEntry(Entry entry, boolean finalizer) { | |||
| if (!finalizer) { | |||
| // recycle now so PoolChunk can be GC'ed. This will only be done if this is not freed because of | |||
| // a finalizer. | |||
| entry.recycle(); | |||
| entry.unguardedRecycle(); | |||
There was a problem hiding this comment.
Is it correctly always previously used by another (atomic) operation that shield it?
Given that the previous one isn't visible is not easy to tell by reading that snippet of code
There was a problem hiding this comment.
@franz1981 It seems this snippet of code is always called by the thread which consumes the MemoryRegionCache.queue, and the MemoryRegionCache.queue is a Mpsc queue, so I think it's safe to use entry.unguardedRecycle() here.
There was a problem hiding this comment.
who consumes the MemoryRegionCache.queue
who consume or offer vs it?
Because consumption is (although it is an impl detail of JMM, actually, so take it with the grain of salt):
slot read item
loadload + loadstore (load-acquire the slot element)
storestore + loadstore (store-release the null value in the slot element)
set slot to null
// and potentially
storestore + loadstore (store-release the entry)
unguarded release
which should end up to
In short, the ordering it's not a problem, concurrency can be, but given that it's a single-consumer queue I see no harm, although it's brittle because it's not immediately visible in the method signature.
Did you have any benchmark (or micro) which show that as an hot spot we care about?
There was a problem hiding this comment.
@franz1981
The thread which releases byteBuf offer the MemoryRegionCache.queue.
I have not done the benchmark yet.
I noticed this because there is a similar entry.unguardedRecycle() calling in method PoolThreadCache.MemoryRegionCache.allocate:
entry.unguardedRecycle() method.
In current implementation, the entry is either recycled by the thread which created it, or by the thread which consumed the Mpsc queue, if the two threads are not the same, then the two threads are mutually exclusive to recycle the entry.
Every thread gets the entry from threadLocal, so there should be no concurrency problem.
There was a problem hiding this comment.
I think we should first really benchmark and see if this makes any difference
|
Let me close this for now |
Motivation:
This is related to #13220, I think we should use
entry.unguardedRecycle()instead ofentry.recycle()in methodMemoryRegionCache.freeEntry(...), to make the 'recycle' operation cheaper.In current implementation, the
entryis either recycled by the thread which created it, or by the thread which consumed the 'Mpsc'MemoryRegionCache.queuequeue, if the two threads are not the same, then the two threads are mutually exclusive to recycle theentry.Every thread gets the
entryfrom the threadLocal, so there should be no concurrency/visible problem.Modification:
Use
entry.unguardedRecycle()instead ofentry.recycle()in methodMemoryRegionCache.freeEntry(...).Result:
Make the 'recycle' operation cheaper.