-
-
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
Reduce ByteBuffer duplication when resizing pooled direct ByteBufs #9765
Conversation
Can one of the admins verify this patch? |
@netty-bot test this please |
@@ -402,7 +402,9 @@ void reallocate(PooledByteBuf<T> buf, int newCapacity, boolean freeOldMemory) { | |||
buf.trimIndicesToCapacity(newCapacity); | |||
bytesToCopy = newCapacity; | |||
} | |||
memoryCopy(oldMemory, oldOffset, buf.memory, buf.offset, bytesToCopy); | |||
if (bytesToCopy > 0) { | |||
oldNioBuffer = memoryCopy(oldMemory, oldNioBuffer, oldOffset, buf, bytesToCopy); |
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.
hmm is this really correct ? This may now be duplicate that is passed to free(...)
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.
@normanmaurer the oldNioBuffer
passed there is already a duplicate (comes from buf.tmpNioBuf
) and isn't free'd itself, rather kept in the cache for later re-use by other ByteBuf
s that share the same "root" nio buffer...
@njhill honestly I wonder if this change really worth it... I mean it makes things more complex and resizing should be "hopefully" not happen too often. Or I am missing anything ? |
@normanmaurer that's a fair point... I noticed this while looking closer at the underlying buffer manipulation done in |
bb789b7
to
34d7054
Compare
@netty-bot test this please |
@njhill can you update the commit message and pr description to match what you did now ? |
Motivation Currently when use of Unsafe is disabled and an internal reallocation is performed for a direct PooledByteBuf, a one-off temporary duplicate is made of the source and destination backing nio buffers so that the copy can be done in a threadsafe manner. The need for this can be reduced by sharing the temporary duplicate buffer that is already stored in the corresponding destination PooledByteBuf instance. Modifications Have PoolArena#memoryCopy(...) take the destination PooledByteBuf instead of the underlying mem reference and offset, and use internalNioBuffer() to obtain/initialize a reusable duplicate of the backing nio buffer. Result Fewer temporary allocations when resizing direct pooled ByteBufs in the non-Unsafe case
34d7054
to
7d0baf6
Compare
@normanmaurer done |
@netty-bot test this please |
@njhill thanks.. and sorry for the delay. |
…9765) Motivation: Currently when use of Unsafe is disabled and an internal reallocation is performed for a direct PooledByteBuf, a one-off temporary duplicate is made of the source and destination backing nio buffers so that the copy can be done in a threadsafe manner. The need for this can be reduced by sharing the temporary duplicate buffer that is already stored in the corresponding destination PooledByteBuf instance. Modifications: Have PoolArena#memoryCopy(...) take the destination PooledByteBuf instead of the underlying mem reference and offset, and use internalNioBuffer() to obtain/initialize a reusable duplicate of the backing nio buffer. Result: Fewer temporary allocations when resizing direct pooled ByteBufs in the non-Unsafe case
Motivation Recent optimization netty#9765 introduced a bug where the native indices of the internal reused duplicate nio buffer are not properly reset prior to using it to copy data during a reallocation operation. This can result in BufferOverflowExceptions thrown during ByteBuf capacity changes. The code path in question applies only to pooled direct buffers when Unsafe is disabled or not available. Modification Ensure ByteBuffer#clear() is always called on the reused internal nio buffer prior to returning it from PooledByteBuf#internalNioBuffer() (protected method); add unit test that exposes the bug. Result Fixes netty#9911
…ize (#9912) Motivation Recent optimization #9765 introduced a bug where the native indices of the internal reused duplicate nio buffer are not properly reset prior to using it to copy data during a reallocation operation. This can result in BufferOverflowExceptions thrown during ByteBuf capacity changes. The code path in question applies only to pooled direct buffers when Unsafe is disabled or not available. Modification Ensure ByteBuffer#clear() is always called on the reused internal nio buffer prior to returning it from PooledByteBuf#internalNioBuffer() (protected method); add unit test that exposes the bug. Result Fixes #9911
…ize (#9912) Motivation Recent optimization #9765 introduced a bug where the native indices of the internal reused duplicate nio buffer are not properly reset prior to using it to copy data during a reallocation operation. This can result in BufferOverflowExceptions thrown during ByteBuf capacity changes. The code path in question applies only to pooled direct buffers when Unsafe is disabled or not available. Modification Ensure ByteBuffer#clear() is always called on the reused internal nio buffer prior to returning it from PooledByteBuf#internalNioBuffer() (protected method); add unit test that exposes the bug. Result Fixes #9911
…ize (netty#9912) Motivation Recent optimization netty#9765 introduced a bug where the native indices of the internal reused duplicate nio buffer are not properly reset prior to using it to copy data during a reallocation operation. This can result in BufferOverflowExceptions thrown during ByteBuf capacity changes. The code path in question applies only to pooled direct buffers when Unsafe is disabled or not available. Modification Ensure ByteBuffer#clear() is always called on the reused internal nio buffer prior to returning it from PooledByteBuf#internalNioBuffer() (protected method); add unit test that exposes the bug. Result Fixes netty#9911
Motivation
Currently when use of Unsafe is disabled and an internal reallocation is performed for a direct
PooledByteBuf
, a one-off temporary duplicate is made of the source and destination backing nio buffers so that the copy can be done in a threadsafe manner.The need for this can be reduced by sharing the temporary duplicate buffer that is already stored in the corresponding destination
PooledByteBuf
instance.Modifications
Have
PoolArena#memoryCopy(...)
take the destinationPooledByteBuf
instead of the underlying mem reference and offset, and useinternalNioBuffer()
to obtain/initialize a reusable duplicate of thebacking nio buffer.
Result
Fewer temporary allocations when resizing direct pooled
ByteBuf
s in the non-Unsafe
case