Skip to content

Commit

Permalink
CompositeByteBuf tidy-up (netty#8784)
Browse files Browse the repository at this point in the history 
Motivation

There's some miscellaneous cleanup/simplification of CompositeByteBuf
which would help make the code a bit clearer.

Modifications

- Simplify web of constructors and addComponents methods, reducing
duplication of logic
- Rename `Component.freeIfNecessary()` method to just `free()`, which is
less confusing (see netty#8641)
- Make loop in addComponents0(...) method more verbose/readable (see
netty#8437 (comment))
- Simplify addition/subtraction in setBytes(...) methods

Result

Smaller/clearer code
  • Loading branch information
@njhill @normanmaurer
njhill authored and normanmaurer committed Feb 1, 2019
1 parent 7bba4f4 commit 98aa5fb
Showing 1 changed file with 40 additions and 54 deletions.
94 changes: 40 additions & 54 deletions buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,7 @@ public CompositeByteBuf(
this(alloc, direct, maxNumComponents,
buffers instanceof Collection ? ((Collection<ByteBuf>) buffers).size() : 0);

addComponents0(false, 0, buffers);
consolidateIfNeeded();
addComponents(false, 0, buffers);
setIndex(0, capacity());
}

Expand Down Expand Up @@ -220,10 +219,7 @@ public CompositeByteBuf addComponent(int cIndex, ByteBuf buffer) {
* {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponent(boolean increaseWriterIndex, ByteBuf buffer) {
checkNotNull(buffer, "buffer");
addComponent0(increaseWriterIndex, componentCount, buffer);
consolidateIfNeeded();
return this;
return addComponent(increaseWriterIndex, componentCount, buffer);
}

/**
Expand Down Expand Up @@ -252,9 +248,7 @@ public CompositeByteBuf addComponents(boolean increaseWriterIndex, ByteBuf... bu
* ownership of all {@link ByteBuf} objects is transferred to this {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponents(boolean increaseWriterIndex, Iterable<ByteBuf> buffers) {
addComponents0(increaseWriterIndex, componentCount, buffers);
consolidateIfNeeded();
return this;
return addComponents(increaseWriterIndex, componentCount, buffers);
}

/**
Expand Down Expand Up @@ -304,14 +298,14 @@ private int addComponent0(boolean increaseWriterIndex, int cIndex, ByteBuf buffe
}
}

// unwrap if already sliced
@SuppressWarnings("deprecation")
private Component newComponent(ByteBuf buf, int offset) {
if (checkAccessible && buf.refCnt() == 0) {
throw new IllegalReferenceCountException(0);
}
int srcIndex = buf.readerIndex(), len = buf.readableBytes();
ByteBuf slice = null;
// unwrap if already sliced
if (buf instanceof AbstractUnpooledSlicedByteBuf) {
srcIndex += ((AbstractUnpooledSlicedByteBuf) buf).idx(0);
slice = buf;
Expand Down Expand Up @@ -345,20 +339,25 @@ public CompositeByteBuf addComponents(int cIndex, ByteBuf... buffers) {
return this;
}

private int addComponents0(boolean increaseWriterIndex, final int cIndex, ByteBuf[] buffers, int arrOffset) {
private CompositeByteBuf addComponents0(boolean increaseWriterIndex,
final int cIndex, ByteBuf[] buffers, int arrOffset) {
final int len = buffers.length, count = len - arrOffset;
// only set ci after we've shifted so that finally block logic is always correct
int ci = Integer.MAX_VALUE;
try {
checkComponentIndex(cIndex);
shiftComps(cIndex, count); // will increase componentCount
ci = cIndex; // only set this after we've shifted so that finally block logic is always correct
int nextOffset = cIndex > 0 ? components[cIndex - 1].endOffset : 0;
for (ByteBuf b; arrOffset < len && (b = buffers[arrOffset]) != null; arrOffset++, ci++) {
for (ci = cIndex; arrOffset < len; arrOffset++, ci++) {
ByteBuf b = buffers[arrOffset];
if (b == null) {
break;
}
Component c = newComponent(b, nextOffset);
components[ci] = c;
nextOffset = c.endOffset;
}
return ci;
return this;
} finally {
// ci is now the index following the last successfully added component
if (ci < componentCount) {
Expand All @@ -379,7 +378,6 @@ private int addComponents0(boolean increaseWriterIndex, final int cIndex, ByteBu

private <T> int addComponents0(boolean increaseWriterIndex, int cIndex,
ByteWrapper<T> wrapper, T[] buffers, int offset) {
checkNotNull(buffers, "buffers");
checkComponentIndex(cIndex);

// No need for consolidation
Expand Down Expand Up @@ -413,18 +411,16 @@ private <T> int addComponents0(boolean increaseWriterIndex, int cIndex,
* {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponents(int cIndex, Iterable<ByteBuf> buffers) {
addComponents0(false, cIndex, buffers);
consolidateIfNeeded();
return this;
return addComponents(false, cIndex, buffers);
}

// TODO optimize further, similar to ByteBuf[] version
// (difference here is that we don't know *always* know precise size increase in advance,
// but we do in the most common case that the Iterable is a Collection)
private int addComponents0(boolean increaseIndex, int cIndex, Iterable<ByteBuf> buffers) {
private CompositeByteBuf addComponents(boolean increaseIndex, int cIndex, Iterable<ByteBuf> buffers) {
if (buffers instanceof ByteBuf) {
// If buffers also implements ByteBuf (e.g. CompositeByteBuf), it has to go to addComponent(ByteBuf).
return addComponent0(increaseIndex, cIndex, (ByteBuf) buffers);
return addComponent(increaseIndex, cIndex, (ByteBuf) buffers);
}
checkNotNull(buffers, "buffers");
Iterator<ByteBuf> it = buffers.iterator();
Expand All @@ -440,12 +436,13 @@ private int addComponents0(boolean increaseIndex, int cIndex, Iterable<ByteBuf>
cIndex = addComponent0(increaseIndex, cIndex, b) + 1;
cIndex = Math.min(cIndex, componentCount);
}
return cIndex;
} finally {
while (it.hasNext()) {
ReferenceCountUtil.safeRelease(it.next());
}
}
consolidateIfNeeded();
return this;
}

/**
Expand Down Expand Up @@ -516,7 +513,7 @@ public CompositeByteBuf removeComponent(int cIndex) {
if (lastAccessed == comp) {
lastAccessed = null;
}
comp.freeIfNecessary();
comp.free();
removeComp(cIndex);
if (comp.length() > 0) {
// Only need to call updateComponentOffsets if the length was > 0
Expand Down Expand Up @@ -547,7 +544,7 @@ public CompositeByteBuf removeComponents(int cIndex, int numComponents) {
if (lastAccessed == c) {
lastAccessed = null;
}
c.freeIfNecessary();
c.free();
}
removeCompRange(cIndex, endIndex);

Expand Down Expand Up @@ -759,7 +756,7 @@ public CompositeByteBuf capacity(int newCapacity) {
c.slice = null;
break;
}
c.freeIfNecessary();
c.free();
bytesToTrim -= cLength;
}
removeCompRange(i + 1, size);
Expand Down Expand Up @@ -1305,15 +1302,11 @@ public int setBytes(int index, InputStream in, int length) throws IOException {
}
}

index += localReadBytes;
length -= localReadBytes;
readBytes += localReadBytes;
if (localReadBytes == localLength) {
index += localLength;
length -= localLength;
readBytes += localLength;
i ++;
} else {
index += localReadBytes;
length -= localReadBytes;
readBytes += localReadBytes;
}
} while (length > 0);

Expand Down Expand Up @@ -1351,15 +1344,11 @@ public int setBytes(int index, ScatteringByteChannel in, int length) throws IOEx
}
}

index += localReadBytes;
length -= localReadBytes;
readBytes += localReadBytes;
if (localReadBytes == localLength) {
index += localLength;
length -= localLength;
readBytes += localLength;
i ++;
} else {
index += localReadBytes;
length -= localReadBytes;
readBytes += localReadBytes;
}
} while (length > 0);

Expand Down Expand Up @@ -1397,15 +1386,11 @@ public int setBytes(int index, FileChannel in, long position, int length) throws
}
}

index += localReadBytes;
length -= localReadBytes;
readBytes += localReadBytes;
if (localReadBytes == localLength) {
index += localLength;
length -= localLength;
readBytes += localLength;
i ++;
} else {
index += localReadBytes;
length -= localReadBytes;
readBytes += localReadBytes;
}
} while (length > 0);

Expand Down Expand Up @@ -1677,7 +1662,7 @@ public CompositeByteBuf discardReadComponents() {
int writerIndex = writerIndex();
if (readerIndex == writerIndex && writerIndex == capacity()) {
for (int i = 0, size = componentCount; i < size; i++) {
components[i].freeIfNecessary();
components[i].free();
}
lastAccessed = null;
clearComps();
Expand All @@ -1689,7 +1674,7 @@ public CompositeByteBuf discardReadComponents() {
// Remove read components.
int firstComponentId = toComponentIndex0(readerIndex);
for (int i = 0; i < firstComponentId; i ++) {
components[i].freeIfNecessary();
components[i].free();
}
lastAccessed = null;
removeCompRange(0, firstComponentId);
Expand All @@ -1715,7 +1700,7 @@ public CompositeByteBuf discardReadBytes() {
int writerIndex = writerIndex();
if (readerIndex == writerIndex && writerIndex == capacity()) {
for (int i = 0, size = componentCount; i < size; i++) {
components[i].freeIfNecessary();
components[i].free();
}
lastAccessed = null;
clearComps();
Expand All @@ -1728,7 +1713,7 @@ public CompositeByteBuf discardReadBytes() {
int firstComponentId = toComponentIndex0(readerIndex);
for (int i = 0; i < firstComponentId; i ++) {
Component c = components[i];
c.freeIfNecessary();
c.free();
if (lastAccessed == c) {
lastAccessed = null;
}
Expand All @@ -1738,7 +1723,7 @@ public CompositeByteBuf discardReadBytes() {
Component c = components[firstComponentId];
if (readerIndex == c.endOffset) {
// new slice would be empty, so remove instead
c.freeIfNecessary();
c.free();
if (lastAccessed == c) {
lastAccessed = null;
}
Expand Down Expand Up @@ -1804,7 +1789,7 @@ void reposition(int newOffset) {
// copy then release
void transferTo(ByteBuf dst) {
dst.writeBytes(buf, idx(offset), length());
freeIfNecessary();
free();
}

ByteBuf slice() {
Expand All @@ -1815,7 +1800,7 @@ ByteBuf duplicate() {
return buf.duplicate().setIndex(idx(offset), idx(endOffset));
}

void freeIfNecessary() {
void free() {
// Release the slice if present since it may have a different
// refcount to the unwrapped buf if it is a PooledSlicedByteBuf
ByteBuf buffer = slice;
Expand All @@ -1824,7 +1809,8 @@ void freeIfNecessary() {
} else {
buf.release();
}
// null out in either case since it could be racy
// null out in either case since it could be racy if set lazily (but not
// in the case we care about, where it will have been set in the ctor)
slice = null;
}
}
Expand Down Expand Up @@ -2130,7 +2116,7 @@ protected void deallocate() {
// We're not using foreach to avoid creating an iterator.
// see https://github.com/netty/netty/issues/2642
for (int i = 0, size = componentCount; i < size; i++) {
components[i].freeIfNecessary();
components[i].free();
}
}

Expand Down

0 comments on commit 98aa5fb

Please sign in to comment.