Re-add pooled sliced buffers?

[ninja-]

77ff24b has deleted the sliced pooled buffer.
@Scottmitch could you re-evaluate this decision. I really liked using slices here and there but I got rid of them to reduce garbage creation after this was reverted.

I am aiming to get rid of copy() etc. again and have another strategy for rewriting packets that would need to expand the buffer.

So instead of operating on a copy

Original (a copy from stream):
-> AAA BBB CCC
rewrite into
-> AAA DDDD CCC

Aiming to do something like this if only the slices are to be recycled again:

Original:
-> AAA BBB CCC

Send:

1. send slice1 AAA
2. send DDDD
3. send slice2 containing CCC

[ninja-]

not to mention the composite buffers :D but I could live without them
(I can just create my own recyclable wrapper and unwrap it at encoder level if I ever need to make a composite out of something)

Oops, closed the issue.

is it really that complicated to properly release them in case the original buffer is an input stream and the slice is made at decoder, fired through the pipeline and later slice.release()d by handler?
I am also going to take care that the slice would never be .retain()ed so it wouldn't block discardSomeReadBytes(). Well, as long as it's fully written to downstream I guess.

I think I can see your point @Scottmitch so how about having two refcnt counters at slices so one of them would just increase the original refcnt and the second one would be dedicated to deciding when to recycle the slice? I hope I am not complicating too much. We could create a common class called DerivedByteBuf we could make DerivedByteBuf take care of maintaing these two refcnts.

[Scottmitch]

@ninja- - I considered fixing the pooling code for derived buffers but it boiled down to having feedback from the PooledByteBuf to all its derived buffers when the reference count went to 0. This would require keeping some sort of collection in PooledByteBuf that must be thread safe. I did not go through this exercise because it seemed like overhead of maintaining this thread safe collection may out weight the benefits of pooling in some/most cases.

That lead me to take the "easier" path which was to pull it out because it was not functioning as originally intended.

Can you explain why copy() is preferred over slice() due to the objects returned by slice() not being pooled?

[ninja-]

Well whether replacing most copy()s with slice() is a premature optimization is an another topic. But what I mean by copy is, well, I forgot to mention that I am not using directly copy().

    ByteBuf dst = ctx.alloc().directBuffer( length );
    in.readBytes( dst );

here's the "copy" as copy() would also use unpooled buffers or something like that.

So using slices avoids memory copies at frame decoder level but would come at a cost of trashing the memory with garbage as there is no pooling. For each single packet read, garbage. And I worked quite hard to pool everything so that the number of garbage it creates during I/O is an exact 0.

IMO that's the behaviour we could use:

1. creating a new derived buffer increases refCnt on the original
2. the derived one maintains its own refCnt starting on 1 and calling retain() on derived would not touch the refCnt on the original
3. if release() is called on derived, the "own refCnt" is decreased. if "own refCnt" is zero, decrease the "original's refCnt" by one. also recycle
4. the user needs to make sure he calls slice.release() so that the original would also be released at some point. but this makes me wonder if that would break buf.slice().copy() since it would start leaking the original buffer...?

But I think that could break things...we need to think about it again knowing that the slices are usually made out of the input stream.
I wouldn't go the slice collection route for the same reasons you mentioned.

[Scottmitch]

@ninja- - If you can make it work without API changes or change in expected behavior then that would be good :) Also be sure to consider the case where there are slice objects of other slice objects.

[ninja-]

@Scottmitch what do you think about the proposed model? the part when I mention that it's hard not to break buf.slice().copy()? Actually the slices now aren't increasing the original refCnt...hmmm :/
I can't even see how the current model is supposed to work. I mean that when I call channel.write(slice) I expect the original buffer to be released if all slices has been written...but clearly this couldn't be the case looking at the code. Am I missing some part of code in derived buffer that actually touches the original refCnt as I can't see any?

[ninja-]

@Scottmitch @normanmaurer ok I see now how it worked. I have an idea how to re-add it, but under another function like rslice() while slice() would stay without ref-counting.
rslice() would increase the parent refcnt by default and decrease when deallocated.
I am struggling to make a class structure that will make both a sliced and reference-counted buffer. any help?

[ninja-]

ok I think I got it...the class structure is quite ugly but it works. basically the derived buffer is refCnted by default and UnpooledSlicedByteBuf contains erasure for refCnting that forwards to parent. PooledSlicedByteBuf extends directly over AbstractSlicedByteBuf

I implemented this with ~ 1000 lines of diff, will PR to review... under rslice() as well as rduplicate() also readRefSlice

[ninja-]

@normanmaurer @Scottmitch fyi https://github.com/ninja-/netty/commits/ninja
works great so far

[normanmaurer]

Fixed by #5144