Improve ByteBufPrimitiveCodec readBytes #1617
Conversation
This method is always used on sliced ByteBuf (as the first 4 bytes old the overall size) so the payload will never match the underlying buffer's array thus defeats the purpose of the optimization and end up costing CPU cycles. On a typical application this represent ~0.4% of the CPU
Directly read the source ByteBuf while checking for content length. Reader index are similarily moved. On a typical application this takes ~0.88% of CPU
| // Move the readerIndex just so we consistently consume the input | ||
| buffer.readerIndex(buffer.writerIndex()); | ||
| return buffer.array(); | ||
| } |
There was a problem hiding this comment.
@chibenwa I'm not sure I understand. Is there a reason why we don't want to use the underlying byte array directly if we can get access to it?
There was a problem hiding this comment.
Because the conditions for reusing the array are never fullfilled.
The buffer layout is 4 bytes size, byyyyyyyyyyyyytes so we would expect the underlying array to match (best case) this layout.
However reading the size means that the leading 4 bytes are read first, and then the payload never matches the underlying array.
Checking if this optimization is doable is impactful enough to be removing it.
Clearer?
There was a problem hiding this comment.
Apologies @chibenwa, I've been out for the holiday here... just getting back to this.
Yeah, your explanation makes sense, thanks for the additional context. I'm reluctant to give up the re-use of the backing array, though, so I put together an alternate implementation which preserves that feature. What do you think of something in that vein?
There was a problem hiding this comment.
The alternate proposal never went anywhere so we're moving forward with the original proposal from @chibenwa
|
I added some tests to better specify the expected behaviour of |
-> Remove a needless call to
ByteBuff::hasArray-> Removes needless slicing