Reduce garbage for requests with unpooled buffers

[danielmitterdorfer]

With this commit we avoiding copying request contents on the HTTP layer
(NIO and Netty 4) when the buffer that holds the request body contents
is unpooled. As the unpooled allocator is usually used on smaller heaps
this helps to reduce garbage further in those situations.

[elasticmachine]

Pinging @elastic/es-core-infra

[danielmitterdorfer]

@jasontedor can you please review the Netty parts?
@tbrooks8 can you please review the NIO parts?

I plan to backport the Netty parts also to 6.x.

[Tim-Brooks]

Theoretically this LGTM. But I think that it will cause the build to fail once #32354 is merged.

I think we always want to release the request? Maybe for the Unpooled version we want request.content().duplicate()? And then release the request?

[Tim-Brooks]

I actually want to point out that this is also not strictly compliant with what is possible in netty. The following is valid:

ByteBuf buffer1 = Unpooled.buffer(10);
ByteBuf buffer = PooledByteBufAllocator.DEFAULT.buffer(10);
CompositeByteBuf byteBufs = new CompositeByteBuf(UnpooledByteBufAllocator.DEFAULT, false, 2, buffer1, buffer);

This creates a composite byte buffer that is backed by a pooled byte buffer. But uses the unpooled allocator. This would be a memory leak in this PR. I don't think this should happen since we tend to only use one allocator type at once. But it is possible. I think maybe we should address this PR by converting the bytes to not be a netty thing?

[Tim-Brooks]

I think maybe we should address this PR by converting the bytes to not be a netty thing?

I guess this is a little tricky right now. So probably not the best approach for this PR.

[danielmitterdorfer]

@tbrooks8 after our discussion yesterday I added an additional assert that checks that all associated buffers are unpooled so we get failures if the assertion is violated. Can you please have a look?

[Tim-Brooks]

LGTM -

Looking at netty - If you set -Dio.netty.allocator.numHeapArenas=0 it looks like the pool allocator delegates to Unpooled.

        if (heapArena != null) {
            buf = heapArena.allocate(cache, initialCapacity, maxCapacity);
        } else {
            buf = PlatformDependent.hasUnsafe() ?
                    new UnpooledUnsafeHeapByteBuf(this, initialCapacity, maxCapacity) :
                    new UnpooledHeapByteBuf(this, initialCapacity, maxCapacity);
        }

netty/netty#1315

We should maybe look to a follow up where we have that set to 0 in the unpooled case. Like we set both the unpooled system property (making it the default) and heap arenas = 0 (making a manual usage of pooled allocator be delegated to unpooled).

An then maybe the logic can be that those settings are enabled?

[danielmitterdorfer]

Thanks for review and good point with the follow-up. When we choose the unpooled allocator ergonomically, this is a pretty simple change (code-wise). When it is set by the user in jvm.options it might be a bit tricky.

[danielmitterdorfer]

I just want to provide an update why I held back merging this change. First of all, I managed to fix this issue for all allocator types (pooled and unpooled) by releasing the buffer at the appropriate place. Then I ran several benchmarks and it turns out that with this change indexing throughput actually decreases by roughly 10% (I tested several of our Rally tracks on 1GB and 4GB heaps).

Profiling reveals the reason: The bulk API uses the buffer's random access API (i.e. #get(index)) to find the next marker (i.e. the new line character for the JSON content type). If we copy the buffer, this is a cheap array access but if we don't, we access a CompositeByteBuffer instance which needs to check which of its components actually contains this index, calculate the offset and then access the buffer element at the right index. I implemented a proof of concept that is optimized for iteration but the results are still worse than with the buffer copy. So I think we should not merge the Netty parts of this PR.

As a next step I want to benchmark the performance impact on our NIO implementation but I'll wait until #32757 is merged.

[danielmitterdorfer]

After running several benchmarks also for the NIO implementation with different workloads and heap sizes it turns out that we do not get a practical benefit from avoiding this copy. Therefore, I'm closing the PR unmerged.