Motivation:
In order to prevent a regression, add test case for a bug that caused a CompositeByteBuf to not release its components.
Modifications:
Add a test case that asserts a CompositeByteBuf's component buffers have indeed been released.
Result:
AbstractCompositeByteBuf gains a test case that will prevent future regressions.
Allow users of Netty to plug in their own leak detector for the purpose
of instrumentation.
Motivation:
We are rolling out a large Netty deployment and want to be able to
track the amount of leaks we're seeing in production via custom
instrumentation. In order to achieve this today, I had to plug in a
custom `ByteBufAllocator` into the bootstrap and have it initialize a
custom `ResourceLeakDetector`. Due to these classes mostly being marked
`final` or having private or static methods, a lot of the code had to
be copy-pasted and it's quite ugly.
Modifications:
* I've added a static loader method for the `ResourceLeakDetector` in
`AbstractByteBuf` that tries to instantiate the class passed in via the
`-Dio.netty.customResourceLeakDetector`, otherwise falling back to the
default one.
* I've modified `ResourceLeakDetector` to be non-final and to have the
reporting broken out in to methods that can be overridden.
Result:
You can instrument leaks in your application by just adding something
like the following:
```java
public class InstrumentedResourceLeakDetector<T> extends
ResourceLeakDetector<T> {
@Monitor("InstanceLeakCounter")
private final AtomicInteger instancesLeakCounter;
@Monitor("LeakCounter")
private final AtomicInteger leakCounter;
public InstrumentedResourceLeakDetector(Class<T> resource) {
super(resource);
this.instancesLeakCounter = new AtomicInteger();
this.leakCounter = new AtomicInteger();
}
@Override
protected void reportTracedLeak(String records) {
super.reportTracedLeak(records);
leakCounter.incrementAndGet();
}
@Override
protected void reportUntracedLeak() {
super.reportUntracedLeak();
leakCounter.incrementAndGet();
}
@Override
protected void reportInstancesLeak() {
super.reportInstancesLeak();
instancesLeakCounter.incrementAndGet();
}
}
```
Motivation:
See #82.
Modifications:
- Added `isText` to validate if the given ByteBuf is compliant with the specified charset.
- Optimized for UTF-8 and ASCII. For other cases, `CharsetDecoder.decoder` is used.
Result:
Users can validate ByteBuf with given charset.
Motivation:
We need to first store a reference to the wrapped buffer before recycle the AbstractPooledDerivedByteBuf instance. This is needed as otherwise it is possible that the same AbstractPooledDerivedByteBuf is again obtained and init(...) is called before we actually have a chance to call release(). This leads to call release() on the wrong buffer.
Modifications:
Store a reference to the wrapped buffer before call recycle and call release on the previous stored reference.
Result:
Always release the correct wrapped buffer when deallocate the AbstractPooledDerivedByteBuf.
Motivation:
If the user uses unsafe direct buffers with no cleaner we can use Unsafe.reallocateMemory(...) as optimization when we need to expand the buffer.
Modifications:
Use Unsafe.relocateMemory(...) in UnpooledUnsafeNoCleanerDirectByteBuf.
Result:
Less expensive expanding of buffers.
Motivation:
Using the Cleaner to release the native memory has a few drawbacks:
- Cleaner.clean() uses static synchronized internally which means it can be a performance bottleneck
- It put more load on the GC
Modifications:
Add new buffer implementations that can be enabled with a system flag as optimizations. In this case no Cleaner is used at all and the user must ensure everything is always released.
Result:
Less performance impact by direct buffers when need to be allocated and released.
Motivation:
Unsafe offers a method to set memory to a specific value. This can be used to implement an optimized version of setZero(...) and writeZero(...)
Modifications:
Add implementation for all Unsafe*ByteBuf implementations.
Result:
Faster setZero(...) and writeZero(...)
Motivation:
At the moment the user is responsible to increase the writer index of the composite buffer when a new component is added. We should add some methods that handle this for the user as this is the most popular usage of the composite buffer.
Modifications:
Add new methods that autoamtically increase the writerIndex when buffers are added.
Result:
Easier usage of CompositeByteBuf.
Motivation:
We missed to override a few methods and so some actions on the ByteBuf failed.
Modifications:
- Override all methods
- Add unit tests to ensure all is fixed.
Result:
All *LeakAware*ByteBuf have correct implementations
Motivation:
Recycler.recycle(...) should not be used anymore and be replaced by Handle.recycle().
Modifications:
Mark it as deprecated and update usage.
Result:
Correctly document deprecated api.
Motivation:
Some tests in PooledByteBufAllocatorTest are blocking on a CountDownLatch. We should use a timeout on these tests so these will not block forever on a failure.
Modifications:
Add timeout param to @Test annotation
Result:
Have sane timeouts on tests.
Motivation:
DefaultByteBufHolder.equals(...) and hashCode() should be implemented so it works correctly with instances that share the same content.
Modifications:
Add implementations and a unit-test.
Result:
Have correctly working equals(...) and hashCode() method
Related: #4333#4421#5128
Motivation:
slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.
Modifications:
- Add the following methods which creates a non-recyclable derived buffer
- retainedSlice()
- retainedDuplicate()
- readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
- a user can replace the content of the holder in a consistent way
- copy/duplicate/retainedDuplicate() can delegate the holder
construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
- Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
- Make ReplayingDecoderByteBuf.reject() return an exception instead of
throwing it so that its callers don't need to add dummy return
statement
Result:
Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
Motivation:
PooledByteBufAllocatorTest.testNumThreadCachesWithNoDirrectArenas() had a race as it just used LockSupport.parkNanos(). We should better use a CountdownLatch and so be sure we really have init everything.
Modifications:
Replace LockSupport.parkNanos(...) with CountdownLatch usage
Result:
No more race in test.
Motivation:
We called deallocationsHuge.decrement() but it needs to be increment()
Modifications:
Replace decrement() with increment()
Result:
Correct metrics.
Motivation:
Often users either need to read or write CharSequences to a ByteBuf. We should add methods for this to ByteBuf as we can do some optimizations for this depending on the implementation.
Modifications:
Add setCharSequence, writeCharSequence, getCharSequence and readCharSequence
Result:
Easier reading / writing of CharSequence with ByteBuf.
Motivation:
Reduce nag warnings when compiling, make it easier for IDEs to display what's deprecated.
Modifications:
Added @Deprecated in a few places
Result:
No more warnings.
Motivation:
We lately added ByteBuf.isReadOnly() which allows to detect if a buffer is read-only or not. We should add ByteBuf.asReadOnly() to allow easily access a read-only version of a buffer.
Modifications:
- Add ByteBuf.asReadOnly()
- Deprecate Unpooled.unmodifiableBuffer(Bytebuf)
Result:
More consistent api.
Motivation:
When FixedCompositeByteBuf was constructed with new ByteBuf[0] and IndexOutOfboundsException was thrown.
Modifications:
Fix constructor
Result:
No more exception
Motivation:
We should not cache the SwappedByteBuf in AbstractByteBuf to reduce the memory footprint.
Modifications:
Not cache the SwappedByteBuf.
Result:
Less memory footprint.
Motivation:
Some ByteBuf implementations do not override all necessary methods,
which can lead to potentially sub-optimal behavior.
Also, SlicedByteBuf does not perform the range check correctly due to
missing overrides.
Modifications:
- Add missing overrides
- Use unwrap() instead of direct member access in derived buffers for
consistency
- Merge unwrap0() into unwrap() using covariant return type
- Deprecate AbstractDerivedByteBuf and its subtypes, because they were
not meant to be public
Result:
Correctness
Motivation:
ByteBuf.readBytes(...) uses Unpooled.buffer(...) internally which will use a heap ByteBuf and also not able to make use of the allocator which may be pooled. We should better make use of the allocator.
Modifications:
Use the allocator for thenew buffer.
Result:
Take allocator into account when copy bytes.
Motiviation:
Sometimes it is useful to dump the status of the PooledByteBufAllocator and log it. Doing this is currently a bit cumbersome as the user needs to basically iterate through all the metrics and compose the String. we would better provide an easy way to do this.
Modification:
Add dumpStats() method.
Result:
Easier to get a view into the status of the allocator.
Motivation:
PoolChunkList.allocate(...) should return false without the need to walk all the contained PoolChunks when the requested capacity is larger then the capacity that can be allocated out of the PoolChunks in respect to the minUsage() and maxUsage() of the PoolChunkList.
Modifications:
Precompute the maximal capacity that can be allocated out of the PoolChunks that are contained in the PoolChunkList and use this to fast return from the allocate(...) method if an allocation capacity larger then that is requested.
Result:
Faster detection of allocations that can not be handled by the PoolChunkList and so faster allocations in general via the PoolArena.
Motivation:
To better understand how much memory is used by Netty for ByteBufs it is useful to understand how many bytes are currently active (allocated) per PoolArena.
Modifications:
- Add PoolArenaMetric.numActiveBytes()
Result:
The user is able to get better insight into the PooledByteBufAllocator.
Motivation:
To make it easier to understand PoolChunk and PoolArena we should cleanup duplicated code.
Modifications:
- Move reused code into methods
- Use Math.max(...)
Result:
Cleaner code and easier to understand.
Motivation:
When doing a normal allocation in PoolArena we also tried to allocate out of the PoolChunkList that only contains completely full PoolChunks. This makes no sense as these are full anyway so an allocation will never work here and just gives a perf hit as we need to walk the whole list of PoolChunks in the list.
Modifications:
Not try to allocate from PoolChunkList that only contains full PoolChunks
Result:
Faster allocation times when a new PoolChunk must be created.
Motivation:
We should better use Math utilities as these are intrinsics. This is a cleanup for ea3ffb8536.
Modifications:
Use Math utilities.
Result:
Cleaner code and use of intrinsics.
Motivation:
When a PoolChunk needs to get moved to the previous PoolChunkList because of the minUsage / maxUsage constraints we always just moved it one level which is incorrect and so could lead to have PoolChunks in the wrong PoolChunkList (in respect to their minUsage / maxUsage settings). This then could have the effect that PoolChunks are not released / freed in a timely fashion and so.
Modifications:
- Correctly move PoolChunks between PoolChunkLists, which includes moving it multiple "levels".
- Add unit test
Result:
Correctlty move the PoolChunk to PoolChunkList when it is freed, even if its multiple layers.
Motivation:
The PoolChunkList.minUsage() and maxUsage() needs to take special action to translate Integer.MIN_VALUE / MAX_VALUE as these are used internal for tail and head of the linked-list structure.
Modifications:
- Correct the minUsage() and maxUsage() methods.
- Add unit test.
Result:
Correct metrics
Motivation:
Sometimes it is useful to allow to disable the leak detection of buffers if the UnpooledByteBufAllocator is used. This is for example true if the app wants to leak buffers into user code but not want to put the burden on the user to always release the buffer.
Modifications:
Add another constructor to UnpooledByteBufAllocator that allows to completely disable leak-detection for all buffers that are allocator out of the UnpooledByteBufAllocator.
Result:
It's possible to disable leak-detection when the UnpooledByteBufAllocator is used.
Motivation:
We should only increment the metric for the huge / normal allocation after it is done. Also we should only decrement once deallocate.
Modifications:
- Move increment after the allocation.
- Fix deallocation metric and move it after deallocation
Result:
More correct metrics.
Motivation:
PoolThreadCache includes the wrong value when throwing a IllegalArgumentException because of freeSweepAllocationThreshold.
Modifications:
Use the correct value.
Result:
Correct exception message.
Motivation:
The method setBytes creates temporary heap buffer when source buffer is read-only.
But this temporary buffer is not used correctly and may lead to data corruption.
This problem occurs when target buffer is pooled and temporary buffer
arrayOffset() is not zero.
Modifications:
Use correct arrayOffset when calling PlatformDependent.copyMemory.
Unit test was added to test this case.
Result:
Setting buffer content works correctly when target is pooled buffer and source
is read-only ByteBuffer.
Motivation:
We also need to add synchronization when access fields to ensure we see the latest updates.
Modifications:
Add synchronization when read fields that are written concurrently.
Result:
Ensure correct visibility of updated.
Motivation:
See #1811
Modifications:
Add LineEncoder and LineSeparator
Result:
The user can use LineEncoder to write a String with a line separator automatically
Motivation:
We had some double spacing in the methods which should be removed to keep things consistent.
Modifications:
Remove redundant spaces.
Result:
Cleaner / consistent coding style.
Motivation:
My previous commit b88a980482 introduced a flawed unit test,
that executes an assertion in a different thread than the test thread.
If this assertion fails, the test doesn't fail.
Modifications:
Replace the assertion by a proper workaround.
Result:
More correct unit test
Motivation:
Circular assignment of arenas to thread caches can lead to less than optimal
mappings in cases where threads are (frequently) shutdown and started.
Example Scenario:
There are a total of 2 arenas. The first two threads performing an allocation
would lead to the following mapping:
Thread 0 -> Arena 0
Thread 1 -> Arena 1
Now, assume Thread 1 is shut down and another Thread 2 is started. The current
circular assignment algorithm would lead to the following mapping:
Thread 0 -> Arena 0
Thread 2 -> Arena 0
Ideally, we want Thread 2 to use Arena 1 though.
Presumably, this is not much of an issue for most Netty applications that do all
the allocations inside the eventloop, as eventloop threads are seldomly shut down
and restarted. However, applications that only use the netty-buffer package
or implement their own threading model outside the eventloop might suffer from
increased contention. For example, gRPC Java when using the blocking stub
performs some allocations outside the eventloop and within its own thread pool
that is dynamically sized depending on system load.
Modifications:
Implement a linear scan algorithm that assigns a new thread cache to the arena
that currently backs the fewest thread caches.
Result:
Closer to ideal mappings between thread caches and arenas. In order to always
get an ideal mapping, we would have to re-balance the mapping whenever a thread
dies. However, that's difficult because of deallocation.
Motivation:
The statistic counters PoolArena.(allocationsTiny|allocationsSmall) are
not protected by a per arena lock, but by a per size class lock. Thus,
two concurrent allocations of different size (class) could lead to a
race and ultimately to wrong statistics.
Modifications:
Use a thread-safe LongCounter instead of a plain long data type.
Result:
Fewer data races.
Motivation:
See #3321
Modifications:
1. Add CharsetUtil.encoder/decoder() methods
2. Deprecate CharsetUtil.getEncoder/getDecoder() methods
Result:
Users can use new CharsetUtil.encoder/decoder() to specify error actions
Motivation:
Utility methods in ByteBufUtil to writeUtf8 and writeAscii expect a buffer to already be allocated. If the user does not have a buffer allocated they have to know details of the encoding in order to know the size of the buffer to allocate.
Modifications:
- Add writeUtf8 and writeAscii which take a ByteBufAllocator and allocate a ByteBuf of the correct size for the user
Result:
ByteBufUtil methods which are easier to use if the user doesn't already have a ByteBuf.
Motivation:
[#4842] introduced 4 new methods but missed to implement advanced leak detection for these.
Modifications:
Correctly implement advanced leak detection for these methods.
Result:
Advanced leak detection works for all methods as expected.
Motivation
See ##3229
Modifications:
Add methods with position independent FileChannel calls to ByteBuf and its subclasses.
Results:
The user can use these new methods to read/write ByteBuff without updating FileChannel's position.
Motivation:
f750d6e36c added support for surrogates in the writeUtf8 conversion. However exceptions are thrown if invalid input is detected, but the JDK (and slow path of writeUtf8) uses a replacement character and does not throw. We should behave the same way.
Modificiations:
- Don't throw in ByteBufUtil.writeUtf8, and instead use a replacement character consistent with the JDK
Result:
ByteBufUtil.writeUtf8 behavior is consistent with the JDK UTF_8 conversion.
Motivation:
We missed to take the byte[] into account when try to access the bytes and so produce a segfault.
Modifications:
Correctly pass the byte[] in.
Result:
No more segfault.
Motivation:
The current interface for CompositeByteBuf.addComponent is not clear under what conditions ownership is transferred when addComponent is called. There should be a well defined behavior so that users can ensure that no leaks occur.
Modifications:
- CompositeByteBuf.addComponent should always assume reference count ownership
Result:
Users that call CompositeByteBuf.addComponent do not have to independently check if the buffer's ownership has been transferred and if not independently release the buffer.
Fixes https://github.com/netty/netty/issues/4760
Motivation:
CompositeByteBuf only implemented simple resource leak detection and how it was implemented was completly different to the way it was for ByteBuf. The other problem was that slice(), duplicate() and others would not return a resource leak enabled buffer.
Modifications:
- Proper implementation for all level of resource leak detection for CompositeByteBuf
Result:
Proper resource leak detection for CompositeByteBuf.
Motivation:
We missed reporting *LE operations when AdvancedLeakAwareByteBuf was used. This could lead to incomplete access reports.
Modifications:
Correctly record access for *LE operations.
Result:
Correct leak reports.
Motivation:
AdvancedLeakAwareByteBuf.forEachByteDesc(...) called recordLeakNonRefCountingOperation() two times which resulted in incorrect leak detection reports.
Modifications:
Remove duplicated call to recordLeakNonRefCountingOperation()
Result:
Correct leak detection results
Motivation:
There are a few buffer leaks related to how Unpooled.wrapped and Base64.encode is used.
Modifications:
- Fix usages of Bas64.encode to correct leaks
- Clarify interface of Unpooled.wrapped* to ensure reference count ownership is clearly defined.
Result:
Reference count code is more clearly defined and less leaks are possible.
Motivation:
Javadoc reports errors about invalid docs.
Modifications:
Fix some errors reported by javadoc.
Result:
A lot of javadoc errors are fixed by this patch.
Motivation:
There are some wrong links and tags in javadoc.
Modifications:
Fix the wrong links and tags in javadoc.
Result:
These links will work correctly in javadoc.
Motivation:
UTF-16 can not represent the full range of Unicode characters, and thus has the concept of Surrogate Pair (http://unicode.org/glossary/#surrogate_pair) where 2 16-bit code units can be used to represent the missing characters. ByteBufUtil.writeUtf8 is currently does not support this and is thus incomplete.
Modifications:
- Add support for surrogate pairs in ByteBufUtil.writeUtf8
Result:
ByteBufUtil.writeUtf8 now supports surrogate pairs and is correctly converting to UTF-8.
Motivation:
We missed to check if the dst is ready only before using unsafe to copy data into it which lead to data-corruption. We need to ensure we respect ready only ByteBuffer.
Modifications:
- Correctly check if the dst is ready only before copy data into it in UnsafeByteBufUtil
- Also make it work for buffers that are not direct and not have an array
Result:
No more data corruption possible if the dst buffer is readonly and unsafe buffer implementation is used.
Motivation:
Initialisation of the ByteBufUtil class, a class frequently used is
delayed because a significant number of String operations is performed to
fill a HEXDUMP_ROWPREFIXES array. This array also sticks to the Strings
forever.
It is quite likely that applications never use the hexdump facility.
Modification:
Moved the static initialisation and references to a static inner class.
This delays initialisation (and memory usage) until actually needed.
The API is kept as is.
Result:
Faster startup time, less memory usage for most netty using applications.
As discussed in #3209, this PR adds Little Endian accessors
to ByteBuf and descendants.
Corresponding accessors were added to UnsafeByteBufUtil,
HeapByteBufferUtil to avoid calling `reverseBytes`.
Deprecate `order()`, `order(buf)` and `SwappedByteBuf`.
Motivation:
The method setBytes did not work correctly because read-only ByteBuffer
does not allow access to its underlying array.
Modifications:
New case was added for ByteBuffer's that are not direct and do not have an array.
These must be handled by copying the data into a temporary array. Unit test was
added to test this case.
Result:
It is now possible to use read-only ByteBuffer as the source
for the setBytes method.
Motivation:
The HTTP/2 RFC (https://tools.ietf.org/html/rfc7540#section-8.1.2) indicates that header names consist of ASCII characters. We currently use ByteString to represent HTTP/2 header names. The HTTP/2 RFC (https://tools.ietf.org/html/rfc7540#section-10.3) also eludes to header values inheriting the same validity characteristics as HTTP/1.x. Using AsciiString for the value type of HTTP/2 headers would allow for re-use of predefined HTTP/1.x values, and make comparisons more intuitive. The Headers<T> interface could also be expanded to allow for easier use of header types which do not have the same Key and Value type.
Motivation:
- Change Headers<T> to Headers<K, V>
- Change Http2Headers<ByteString> to Http2Headers<CharSequence, CharSequence>
- Remove ByteString. Having AsciiString extend ByteString complicates equality comparisons when the hash code algorithm is no longer shared.
Result:
Http2Header types are more representative of the HTTP/2 RFC, and relationship between HTTP/2 header name/values more directly relates to HTTP/1.x header names/values.
Motivation:
Modulo operations are slow, we can use bitwise operation to detect if resource leak detection must be done while sampling.
Modifications:
- Ensure the interval is a power of two
- Use bitwise operation for sampling
- Add benchmark.
Result:
Faster sampling.
Motivation:
Fix a race condition that was introduced by f18990a8a5 that could lead to a NPE when allocate from the PooledByteBufAllocator concurrently by many threads.
Modifications:
Correctly synchronize on the PoolSubPage head.
Result:
No more race.
Motiviation:
We have a lot of duplicated code which makes it hard to maintain.
Modification:
Move shared code to UnsafeByteBufUtil and use it in the implementations.
Result:
Less duplicated code and so easier to maintain.
Motiviation:
We have a lot of duplicated code which makes it hard to maintain.
Modification:
Move shared code to HeapByteBufUtil and use it in the implementations.
Result:
Less duplicated code and so easier to maintain.
Motivation:
sun.misc.Unsafe allows us to handle heap ByteBuf in a more efficient matter. We should use special ByteBuf implementation when sun.misc.Unsafe can be used to increase performance.
Modifications:
- Add PooledUnsafeHeapByteBuf and UnpooledUnsafeHeapByteBuf that are used when sun.misc.Unsafe is ready to use.
- Add UnsafeHeapSwappedByteBuf
Result:
Better performance when using heap buffers and sun.misc.Unsafe is ready to use.
Motivation:
We had a bug in our implemention which double "reversed" bytes on systems which not support unaligned access.
Modifications:
- Correctly only reverse bytes if needed.
- Share code between unsafe implementations.
Result:
No more data-corruption on sytems without unaligned access.
Motivation:
When moving bytes between a PooledUnsafeDirectByteBuf or an UnpooledUnsafeDirectByteBuf
and a ByteBuffer, a temp ByteBuffer is allocated and will need to be GCed. This is a
common case since a ByteBuffer is always needed when reading/writing on a file,
for example.
Modifications:
Use PlatformDependent.copyMemory() to avoid the need for the temp ByteBuffer
Result:
No temp ByteBuffer allocated and GCed.
Motivation:
SlicedByteBuf did double reference count checking for various bulk operations, which affects performance.
Modifications:
- Add package private method to AbstractByteBuf that can be used to check indexes without check the reference count
- Use this new method in the bulk operation os SlicedByteBuf as the reference count checks take place on the wrapped buffer anyway
- Fix test-case to not try to read data that is out of the bounds of the buffer.
Result:
Better performance on bulk operations when using SlicedByteBuf (and sub-classes)
Motivation:
Some of the tests in the buffer module contained unused code. Some of the tests also used unnecessary inheritance which could be avoided to simplify code.
Modifications:
Cleanup the test cases.
Result:
Cleaner code, less cruft.
Motivation:
We need to always return a real slice even when the requested length is 0. This is needed as otherwise we not correctly share the reference count and so may leak a buffer if the user call release() on the returned slice and expect it to decrement the reference count of the "parent" buffer.
Modifications:
- Always return a real slice
- Add unit test for the bug.
Result:
No more leak possible when a user requests a slice of length 0 of a SlicedByteBuf.
