-
-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
Use entry.unguardedRecycle() instead of entry.recycle() in method MemoryRegionCache.freeEntry(...) #13727
Closed
Closed
Use entry.unguardedRecycle() instead of entry.recycle() in method MemoryRegionCache.freeEntry(...) #13727
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franz1981 It seems this snippet of code is always called by the thread which consumes the
MemoryRegionCache.queue
, and theMemoryRegionCache.queue
is aMpsc
queue, so I think it's safe to useentry.unguardedRecycle()
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
which should end up to
netty/common/src/main/java/io/netty/util/Recycler.java
Line 293 in a34f445
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franz1981
The thread which releases
byteBuf
offer theMemoryRegionCache.queue
.I have not done the benchmark yet.
I noticed this because there is a similar
entry.unguardedRecycle()
calling in methodPoolThreadCache.MemoryRegionCache.allocate
:netty/buffer/src/main/java/io/netty/buffer/PoolThreadCache.java
Line 382 in c295086
entry.unguardedRecycle()
method.In current implementation, the
entry
is either recycled by the thread which created it, or by the thread which consumed theMpsc
queue, if the two threads are not the same, then the two threads are mutually exclusive to recycle theentry
.Every thread gets the
entry
from threadLocal, so there should be no concurrency problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should first really benchmark and see if this makes any difference