Server configuration for direct/heap ByteBuffers

[sbordet]

This issue aggregates #183, #226, #351, #2135.

Currently we have a number of configurations for dealing with direct/heap buffers:

• HttpConfiguration.useDirectBuffers - introduced in master: Support Direct Byte Buffers #348 for Jetty 9.x: Support Direct Byte Buffers #351. While I don't think this is a HTTP configuration (it's more a ServerConnector/ConnectionFactory configuration) I guess it's ok to have it here as we also have HttpConfiguration.idleTimeout. Javadocs for HttpConfiguration.useDirectBuffers says it's about requests, but it is in fact applied to responses too. It is only used in HttpConnection so not for SSL and neither for HTTP/2 nor WebSocket.
• EndPoint.isOptimizedForDirectBuffers() - this is a getter only property that supposedly tells whether the EndPoint works better with direct buffers. I think there are wrong assumptions here, such as "TLS EndPoints will work better with heap buffers" which may be true when using the JDK TLS implementation (and even then it's questionable if it really is), but it's wrong when we use native implementations such as Conscrypt. I believe this getter should be completely removed since it's based on wrong assumptions.
• HttpTransport.isOptimizedForDirectBuffers() - write side only and trickled up from EndPoint, same observations, should be removed.
• HttpOutput.Interceptor.isOptimizedForDirectBuffers() - ditto.
• HttpChannel.isOptimizedForDirectBuffers() - ditto. This is inherited from HttpOutput.Interceptor.
• HttpChannel.useDirectBuffers() - this is a getter only property that is a duplicate in functionality for HttpChannel.isOptimizedForDirectBuffers() and it's used in the write side by HttpOutput so now HttpChannel has 2 getters for the same thing and HttpOutput looks sometimes at one and sometimes at the other - should be removed.
• SslConnectionFactory.directBuffersFor[Encryption|Decryption] - introduced by Android 8.1 needs direct buffers for SSL/TLS to work #2135 used only by SslConnection which defeats EndPoint.isOptimizedForDirectBuffers().

[sbordet]

On the read side, the place where we need to know whether to use heap/direct buffers is typically from AbstractConnection.onFillable().
This means that either the Connection concrete class is constructed with that boolean heap/direct value (like SslConnection is), or needs a way to retrieve it (currently HttpConnection uses HttpConfiguration while HTTP2Connection hardcodes "heap" 😭).

On the write side, the place where we need to know whether to use heap/direct buffers is typically in the generation phase, where we take some application content (could be heap or direct) and we need to generate some additional framing around it.
For example:

HttpOutput.write(byte[]) // writes to aggregate buffer or it's wrapped
  HttpOutput.Interceptor.write(ByteBuffer, boolean, Callback)
    HttpChannel.write(ByteBuffer, boolean, Callback)
      HttpTransport.send(..., ByteBuffer, boolean, Callback)

HttpOutput._aggregate uses HttpOutput.Interceptor.isOptimizedForDirectBuffers(), which may be heap (e.g. GZIP interceptor) or direct or mixed.
HTTP/1.1 (HttpConnection) uses HttpConfiguration to decide heap/direct for the framing.
HTTP/2 hardcodes direct for the frame header bytes but hardcodes heap for the hpack bytes.

[sbordet]

My proposal is the following:

• Introduce HttpConfiguration.use[Input|Output]DirectBuffers - remove/deprecate the existing useDirectBuffers and introduce 2 to split input from output. This is the most coarse configuration.
• Introduce similar getters and setters for ConnectionFactory implementations that take HttpConfiguration at construction time (not all do, e.g. SslConnectionFactory). This will allow to override the value "inherited" from HttpConfiguration via setters on the ConnectionFactory.
• Introduce similar getters and setters for Connection implementations that need these settings. Their respective ConnectionFactory will pass over their settings when constructing the concrete Connection implementation. The value can be further customized for the specific Connection. This guarantees that AbstractConnection.onFillable() can be implemented using useInputDirectBuffers taken directly from the Connection itself.
• Other components such as HttpOutput will retrieve the value from the Connection; HttpTransport implementation will also have access to the Connection to retrieve useOutputDirectBuffers.
• GZIP interceptors either have a reference to the HttpChannel which has a way to access the Connection (or be configured by the Connection), or can be changed to take the boolean heap/direct additional parameter.

To remove:

• EndPoint.isOptimizedForDirectBuffers()
• HttpConnection.isOptimizedForDirectBuffers()
• HttpTransport.isOptimizedForDirectBuffers()
• HttpOutput.isOptimizedForDirectBuffers()
• HttpChannel.useDirectBuffers()

To change:

• SslConnectionFactory already has the proper configuration, just under a different name: useDirectBuffersFor[Encryption|Decryption] - I think it can be left as is or maybe deprecated to have the same meaning of use[Input|Output]DirectBuffers.

All of the above is proposed for Jetty 10 only, where we can remove methods.
@gregw @joakime thoughts?

[gregw]

I think that if we are to remove the isOptimizedForDirectBuffers methods implies that we really know if direct buffers are better or worse. If we really do know that, then why make direct buffers configurable in any way - we should just use what is best.

The problem is that I don't think we yet really understand the pros/cons of using Direct buffers. I'm sure that the assumptions about them that existed when this code was written have changed a lot, so we definitely need to update this code. But the assumption now appears to be that using direct buffers now is all pro and no con? If it is true (and I'm dubious), then we should just remove the ability to configure it at all.

So I think further research is need for us to better understand the current tradeoffs before we propose any API changes. We need to hurry up with this as the door for major API changes in 10 is closing

[sbordet]

I think that if we are to remove the isOptimizedForDirectBuffers methods implies that we really know if direct buffers are better or worse. If we really do know that, then why make direct buffers configurable in any way - we should just use what is best.

We know that direct buffers are not much different from heap buffers to read one byte at a time. We know they are better for raw reads and raw writes because there is one less copy involved (done by the JVM).
We want to make them configurable because there is people that, no matter what, do not want to use direct buffers (e.g. everything in heap so they can constrain better how much OS memory the JVM uses), or because Conscrypt had bugs that forced the use of direct buffers, and because we want to give configurability in general.

I don't know think we need further research - we know read wise they are comparable, and write wise direct buffers perform less copies.
This issue will give overridable options consistently used across Jetty (not only in HTTP/1.1).
I will have a PR soon.

[gregw]

OK - I'm still dubious that it is all upside for direct buffers.... but if we can mostly run with them then getting rid of the isOptimized... methods will be a good code volume/complexity reduction. Having them configurable will give us a course grained switch to turn them off if (when) we discover problems with them.