Review temporary buffer usage

[gregw]

Jetty version
9.4.x, 10.0.x

Description

See #5045 for some background. After looking at that issue, it is apparent that when we fail to do all our writing as channels, we are probably doing a very bad job in regards to too many copies into temporary buffers and not reusing those temporary buffers:

• The HttpContentRangeWriter is preferring a direct buffer, but ByteBufferRangeWriter always does a writeTo, causing a copy to a temp buffer. Even if an indirect buffer is provided, it is likely to be a readonly buffer and same problem.
• CachedHttpContent always returns a readOnlyBuffer for in memory content. This prevents access to the underlying array and thus can cause more copying. We should consider the risks of returning a normal buffer... or making our own readonly wrapper that still exposes the array.
• SeekableByteChannelRangeWriter needs to be reviewed. If we can only write ranges as byte[] then reading them as ByteBuffers will only require yet another copy to the byte array.

[gregw]

Also: https://github.com/eclipse/jetty.project/blob/6d88f966e1bfe910f03ac1179ee84877c47d01e2/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/message/SimpleBinaryMessage.java#L61

[l0rinc]

We've bumped into this issue recently, any plans to tackle this in the near future?

[lachlan-roberts]

@paplorinc Yes, I have been working on a PR for this, although it will probably not make 9.4.31.
This is a general review of temporary buffer usage, can you provide more info about the issue you had, is there a specific case that was causing you concern and you would like addressed in the PR?

[l0rinc]

Hey Lachlan, we'd like to reuse buffers when sending and receiving binary messages, i.e. onWebSocketBinary, but with a ByteBuffer to avoid the GC penalty for many small messages.

[lachlan-roberts]

@paplorinc PR #5045 should improve the temporary buffer usage for small binary messages. But we are still not reusing the buffers.

We use ByteArrayOutputStream to aggregate the payloads of a frames making up a websocket message. I will investigate improving this in jetty-10, maybe by having a version of ByteArrayOutputStream which uses the jetty ByteBufferPool.

In the case of a single frame binary message, we should be able to avoid the ByteArrayOutputStream entirely and just use the ByteBuffer from the websocket frame directly, this would avoid the buffer allocation and the copying. I think I have made this optimisation in Jetty-10 but have not brought it back to 9.4.

[l0rinc]

Hey Lachlan, thanks for the update.
I saw that Jetty was using a ByteArrayOutputStream to aggregate the continuation frames to a final buffer in SimpleBinaryMessage.
What if instead of that, we returned a blocking inputstream, which would fetch until isFin is true, reading from the underlying, read-only ByteBuffers?
Also, do you consider the 10x or 11x branches stable for production? If so, how come it's an alpha?

[lachlan-roberts]

What if instead of that, we returned a blocking inputstream, which would fetch until isFin is true, reading from the underlying, read-only ByteBuffers?

Take a look at MessageInputStream, this will allow you to declare an @OnWebSocketMessage annotation to receive an InputStream. Looks to not be as efficient in 9.4.x either, I can see that it is still copying the payload which it is not doing in 10.0.x. We might be able to remove the copying after some recent changes in PR #4486.

Also, do you consider the 10x or 11x branches stable for production? If so, how come it's an alpha?

Jetty jetty-10.0.x is in beta right now we are expecting to put out the first official release in the next week or so. It is the main development branch now and includes a major rewrite of the websocket code. So we are prioritising the jetty 10 branch for major work.

[l0rinc]

So we are prioritising the jetty 10 branch for major work.

Unfortunately we need JDK 8 compatibility - is it possible to backport the buffer usage optimizations to 9.x?

[gregw]

@paplorinc Jetty-9 is in feature freeze other than for sponsored work for our commercial clients. We have to give priority to stability in 9 rather than features, so we now do not back port work from 10 unless there is good reason to, especially with websocket, where the implementations are significantly different and the risk of breaking something is high.

Thus we are unlikely to do this ourselves. However, if you wanted to prepare a PR for jetty-9, then we would consider it, but there will still be a high bar for inclusion for any significant changes that risk stability.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[github-actions]

This issue has been closed due to it having no activity.