Improve temporary buffer usage for WebSocket PerMessageDeflate.

[leonchen83]

Jetty version
9.4.x
Java version
java-11
OS type/version
unix
Description

CompressExtension.decompress method

protected void decompress(ByteAccumulator accumulator, ByteBuffer buf) throws DataFormatException
    {
        if ((buf == null) || (!buf.hasRemaining()))
        {
            return;
        }
        byte[] output = new byte[DECOMPRESS_BUF_SIZE];

        ...
    }

we use async-profiler to profile memory allocate and found that
byte[] output = new byte[DECOMPRESS_BUF_SIZE]; cosume too many memory as following

could we create that byte[] use thread local like

private static ThreadLocal<byte[]> BYTE = ThreadLocal.withInitial(() -> new byte[DECOMPRESS_BUF_SIZE]);
protected void decompress(ByteAccumulator accumulator, ByteBuffer buf) throws DataFormatException
    {
        if ((buf == null) || (!buf.hasRemaining()))
        {
            return;
        }
        byte[] output = BYTE.get();

        ...
    }

[gregw]

@lachlan-roberts can you write a comment saying the status of your work to pool buffers used for gzip - that's mostly jetty-10 right??

What can be done in 9.4 and is this approach compatible... at least for 9?

[lachlan-roberts]

The 9.4 implementation is quite inefficient and I don't think the ThreadLocal would improve it much. We are reading into this temporary byte array, but then copy this into a ByteAccumulator with another byte array allocation, the data in the ByteAccumulator is eventually transfered to a ByteBuffer taken from the ByteBufferPool.

There was a big rework of the CompressExtension / PerMessageDeflateExtension in Jetty 10 and now we use the ByteBufferPool to get buffers for this. We use frame fragmentation, we decompress directly into a ByteBuffer taken from the pool and then forward this on before decompressing any more. So we avoid all this unnecessary copying and byte array allocations.

I don't think it would be very easy to bring back these changes to 9.4.x. It would require lots of changes specific to 9.4 for the CompressExtension and PerMessageDeflateExtension.

[gregw]

Thanks @lachlan-roberts. So it doesn't sound that we can back port the fixes.... but then it also sounds like 9.4 has some low hanging fruit that could offer some improvements.
The byte accumulator class looks like it could be rewritten.... so that either it is a chain of temporary byte arrays that are copied once into a pooled byte buffer... or perhaps it can accumulate directly into a ByteBuffer, only copying (or chaining) if the capacity is exceeded.

So let's keep this open and once 10 is out the door (priority) see if there is something we can do.

[leonchen83]

@gregw @lachlan-roberts
hi I made a PR5520 to improve this issue.
could you help me to review that code. If that PR5520 can't merge to branch for some reason, that is OK , hope some idea in that PR5520 that can help you to improve this issue.

[joakime]

Fixing memory is a good goal.

But using a ThreadLocal is not an option here.

The WebSocket implementation in Jetty is asynchronous, meaning that for any websocket message it's very probable that more than 1 thread is used to read from network, parse websocket frames, send frame through extension stack, etc ...

[joakime]

@gregw @lachlan-roberts
hi I made a PR5520 to improve this issue.
could you help me to review that code. If that PR5520 can't merge to branch for some reason, that is OK , hope some idea in that PR5520 that can help you to improve this issue.

Most Important: Fix your ECA, that PR can never be merged unless you have a proper ECA.

[leonchen83]

@lachlan-roberts @gregw @joakime
after fix the memory allocation is following

[lachlan-roberts]

Closing as PR #5574 has been merged and has improved the memory allocation from PerMessageDeflateExtension.