Fix for incorrect values from CompositeByteBuf#component(int)#9416
Fix for incorrect values from CompositeByteBuf#component(int)#9416
Conversation
|
Can one of the admins verify this patch? |
|
Performance comparison looks good |
|
@njhill Reviewed. Thank you for your patch! |
|
@njhill I'm sorry but... when can I use this patch? |
|
@jingene it still needs additional review but hopefully will be in the next release |
|
@netty-bot test this please |
|
@njhill I wonder if all the complexity really worth it and if we should not better remove some optimisations to make the code easier to reason about.... |
|
@normanmaurer sure I'll have a go at that |
|
@njhill let me know once there is something I should review / re-review. |
|
@normanmaurer sure, hopefully I'll get to it in the next day or 2 |
|
@njhill ping.. |
|
@jingene apologies, juggling bunch of things, will try to do it today. Would def like for this to get into the next release. |
Motivation The way that Components are added/stored in CompositeByteBuf was changed in netty#8437 to minimize slicing and runtime access indirection, with the obvious intention to preserve the effective behaviour of public methods. @jingene discovered a discrepancy, reported in netty#9398, where the component(int) and componentAtOffset(int) methods can return an incorrect ByteBuf, i.e. not equivalent to a slice of the added buffer at the time it was added (the previous behaviour). This can happen in particular if the added ByteBuf is a wrapped slice, e.g. a leak aware or unreleasable buffer. Upon further scrutiny another subtle deviation was also noticed: When the added `ByteBuf is a slice whose readable region does not cover the whole buffer internalComponent(int) returns the original slice (with capacity != readableBytes) rather than a sliced slice. Modifications Unfortunately to fix this robustly required slightly more invasive change than first expected. - A final ByteBuf srcBuf field has been added to the Component class, which always holds the originally-added buffer. This simplifies some of the existing fragile logic where we need access to this (e.g. when releasing) - Component has been made abstract with two final impls. Which of these is used depends on whether the srcBuf is already a slice of the component's range (or equivalent to one) - Correctly handle the various places where access to the pre-unwrapped buffer is important, including the problem component(int) methods - Extend the pre-unwrapping optimization to cover WrappedByteBufs, SwappedByteBufs, and duplicates in addition to slices - Unit test for the original bug provided by @jingene Result - Correct behaviour of the (internal)component(AtOffset) methods in all cases - Reduced indirection when accessing components that were added from more kinds of ByteBufs (duplicates in particular) - Less fragile logic related to lazy-slicing components I don't expect there to be any noticeable performance impact of splitting Component into two subclasses since most of the methods are still final in the superclass and the ones that aren't will benefit from bimorphic inlining. But will aim to benchmark nonetheless. There could be slight increase in mem use due to two additional fields in _one_ of the two Component subtypes, but I think this is needed for correctness and still much better than slicing components unconditionally when adding them. Co-authored-by: jingene <jingene0206@gmail.com>
77ecc66 to
434b6e6
Compare
|
@normanmaurer I have pushed a commit with some simplification, that hopefully makes things a little clearer. Unfortunately most of the changes are for correctness than performance. For example the In either case I do think it makes sense to deal differently with added buffers whose entire capacity is readable since these would be quite common and don't need to be sliced at any stage. |
|
@netty-bot Test this please |
|
@njhill I see... sure saving all the allocations is quite nice but looking at the code it seems like a maintainance nightmare which just waits to break once we change some internals :/ |
|
@normanmaurer that's a fair comment! Are you referring to this PR or the current
|
|
@normanmaurer @jingene I've opened #9525 in place this, PTAL! |
Motivation This is a "simpler" alternative to netty#9416 which fixes the same CompositeByteBuf bugs described there, originally reported by @jingene in netty#9398. Modifications - Add fields to Component class for the original buffer along with its adjustment, which may be different to the already-stored unwrapped buffer. Use it in appropriate places to ensure correctness and equivalent behaviour to that prior to the earlier optimizations - Add comments explaining purpose of each of the Component fields - Unwrap more kinds of buffers in newComponent method to extend scope of the existing indirection-reduction optimization - De-duplicate common buffer consolidation logic - Unit test for the original bug provided by @jingene Result - Correct behaviour / fixed bugs - Some code deduplication / simplification - Unwrapping optimization applied to more types of buffers The downside is increased mem footprint from the two new fields, and additional allocations in some specific cases, though those should be rare. Co-authored-by: jingene <jingene0206@gmail.com>
Motivation This is a "simpler" alternative to #9416 which fixes the same CompositeByteBuf bugs described there, originally reported by @jingene in #9398. Modifications - Add fields to Component class for the original buffer along with its adjustment, which may be different to the already-stored unwrapped buffer. Use it in appropriate places to ensure correctness and equivalent behaviour to that prior to the earlier optimizations - Add comments explaining purpose of each of the Component fields - Unwrap more kinds of buffers in newComponent method to extend scope of the existing indirection-reduction optimization - De-duplicate common buffer consolidation logic - Unit test for the original bug provided by @jingene Result - Correct behaviour / fixed bugs - Some code deduplication / simplification - Unwrapping optimization applied to more types of buffers The downside is increased mem footprint from the two new fields, and additional allocations in some specific cases, though those should be rare. Co-authored-by: jingene <jingene0206@gmail.com>
Motivation This is a "simpler" alternative to #9416 which fixes the same CompositeByteBuf bugs described there, originally reported by @jingene in #9398. Modifications - Add fields to Component class for the original buffer along with its adjustment, which may be different to the already-stored unwrapped buffer. Use it in appropriate places to ensure correctness and equivalent behaviour to that prior to the earlier optimizations - Add comments explaining purpose of each of the Component fields - Unwrap more kinds of buffers in newComponent method to extend scope of the existing indirection-reduction optimization - De-duplicate common buffer consolidation logic - Unit test for the original bug provided by @jingene Result - Correct behaviour / fixed bugs - Some code deduplication / simplification - Unwrapping optimization applied to more types of buffers The downside is increased mem footprint from the two new fields, and additional allocations in some specific cases, though those should be rare. Co-authored-by: jingene <jingene0206@gmail.com>
Motivation
The way that Components are added/stored in
CompositeByteBufwas changed in #8437 to minimize slicing and runtime access indirection, with the obvious intention to preserve the effective behaviour of public methods.@jingene discovered a discrepancy, reported in #9398, where the
component(int)andcomponentAtOffset(int)methods can return an incorrectByteBuf, i.e. not equivalent to a slice of the added buffer at the time it was added (the previous behaviour). This can happen inparticular if the added
ByteBufis a wrapped slice, e.g. a leak aware or unreleasable buffer.Upon further scrutiny another subtle deviation was also noticed: When the added
ByteBufis a slice whose readable region does not cover the whole bufferinternalComponent(int)returns the original slice (with capacity != readableBytes) rather than a sliced slice.Modifications
Unfortunately to fix this robustly required slightly more invasive change than first expected.
final ByteBuf srcBuffield has been added to theComponentclass, which always holds the originally-added buffer. This simplifies some of the existing fragile logic where we need access to this (e.g. when releasing)Componenthas been made abstract with two final impls. Which of these is used depends on whether thesrcBufis already a slice of the component's range (or equivalent to one)component(int)methodsWrappedByteBufs,SwappedByteBufs, and duplicates in addition to slicesResult
(internal)component(AtOffset)methods in all casesByteBufs (duplicates in particular)I don't expect there to be any noticeable performance impact of splitting Component into two subclasses since most of the methods are still final in the superclass and the ones that aren't will benefit from bimorphic inlining. But will aim to benchmark nonetheless.
There could be slight increase in mem use due to two additional fields in one of the two
Componentsubtypes, but I think this is needed for correctness and still much better than slicing components unconditionally when adding them.Co-authored-by: jingene jingene0206@gmail.com