Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reinstate HttpChannel reuse in H2 #10868

Merged
merged 5 commits into from
Nov 14, 2023
Merged

reinstate HttpChannel reuse in H2 #10868

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2c253f
reinstate HttpChannel reuse in H2
gregw Nov 9, 2023
59e89dc
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12.…
joakime Nov 9, 2023
30ba4e7
add comments and remove boolean flag
lorban Nov 13, 2023
4130d2f
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12.…
lorban Nov 13, 2023
dbcf2e3
Restored getter/setter for recycling HttpChannels.
sbordet Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 18 additions & 60 deletions ...2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
import java.util.Base64;
import java.util.List;
import java.util.Objects;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.http.BadMessageException;
Expand Down Expand Up @@ -60,12 +62,14 @@ public class HTTP2ServerConnection extends HTTP2Connection implements Connection
private static final Logger LOG = LoggerFactory.getLogger(HTTP2ServerConnection.class);

private final HttpChannel.Factory httpChannelFactory = new HttpChannel.DefaultFactory();
private final Queue<HttpChannel> httpChannels = new ConcurrentLinkedQueue<>();
sbordet marked this conversation as resolved.
Show resolved Hide resolved
private final Attributes attributes = new Lazy();
private final List<Frame> upgradeFrames = new ArrayList<>();
private final Connector connector;
private final ServerSessionListener listener;
private final HttpConfiguration httpConfig;
private final String id;
private boolean recycleHttpChannels = true;

public HTTP2ServerConnection(Connector connector, EndPoint endPoint, HttpConfiguration httpConfig, HTTP2ServerSession session, int inputBufferSize, ServerSessionListener listener)
{
Expand Down Expand Up @@ -118,7 +122,7 @@ public void onNewStream(HTTP2Stream stream, HeadersFrame frame)
if (LOG.isDebugEnabled())
LOG.debug("Processing {} on {}", frame, stream);

HttpChannel httpChannel = httpChannelFactory.newHttpChannel(this);
HttpChannel httpChannel = pollHttpChannel();
HttpStreamOverHTTP2 httpStream = new HttpStreamOverHTTP2(this, httpChannel, stream);
httpChannel.setHttpStream(httpStream);
stream.setAttachment(httpStream);
Expand Down Expand Up @@ -223,83 +227,37 @@ public void push(HTTP2Stream stream, MetaData.Request request)
if (LOG.isDebugEnabled())
LOG.debug("Processing push {} on {}", request, stream);

HttpChannel httpChannel = httpChannelFactory.newHttpChannel(this);
HttpChannel httpChannel = pollHttpChannel();
HttpStreamOverHTTP2 httpStream = new HttpStreamOverHTTP2(this, httpChannel, stream);
httpChannel.setHttpStream(httpStream);
Runnable task = httpStream.onPushRequest(request);
if (task != null)
offerTask(task, true);
}

/*
// TODO: re-instate recycle channel functionality, but using Pool.
private final AutoLock lock = new AutoLock();
private final Queue<HttpChannelOverHTTP2> channels = new ArrayDeque<>();
private boolean recycleHttpChannels = true;

public boolean isRecycleHttpChannels()
{
return recycleHttpChannels;
}

public void setRecycleHttpChannels(boolean recycleHttpChannels)
private HttpChannel pollHttpChannel()
{
this.recycleHttpChannels = recycleHttpChannels;
HttpChannel httpChannel = isRecycleHttpChannels() ? httpChannels.poll() : null;
if (httpChannel == null)
httpChannel = httpChannelFactory.newHttpChannel(this);
return httpChannel;
}

private HttpChannelOverHTTP2 provideHttpChannel(Connector connector, IStream stream)
void offerHttpChannel(HttpChannel channel)
{
HttpChannelOverHTTP2 channel = pollHttpChannel();
if (channel != null)
{
channel.getHttpTransport().setStream(stream);
if (LOG.isDebugEnabled())
LOG.debug("Recycling channel {} for {}", channel, this);
}
else
{
HttpTransportOverHTTP2 transport = new HttpTransportOverHTTP2(connector, this);
transport.setStream(stream);
channel = newServerHttpChannelOverHTTP2(connector, httpConfig, transport);
channel.setUseOutputDirectByteBuffers(isUseOutputDirectByteBuffers());
if (LOG.isDebugEnabled())
LOG.debug("Creating channel {} for {}", channel, this);
}
stream.setAttachment(channel);
return channel;
}

protected ServerHttpChannelOverHTTP2 newServerHttpChannelOverHTTP2(Connector connector, HttpConfiguration httpConfig, HttpTransportOverHTTP2 transport)
{
return new ServerHttpChannelOverHTTP2(connector, httpConfig, getEndPoint(), transport);
if (isRecycleHttpChannels())
httpChannels.offer(channel);
}

private void offerHttpChannel(HttpChannelOverHTTP2 channel)
public boolean isRecycleHttpChannels()
{
if (isRecycleHttpChannels())
{
try (AutoLock l = lock.lock())
{
channels.offer(channel);
}
}
return recycleHttpChannels;
}

private HttpChannelOverHTTP2 pollHttpChannel()
public void setRecycleHttpChannels(boolean recycleHttpChannels)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config needs to be wired?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I took this from the commented out previous implementation. I think it can be permanently on and not wired into config. It is only there as an option in case we suspect a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if it's not wired, it's useless as there is no way a user can enable it, and we can easily remove this recycling logic locally if we ever suspect it's causing problems and are working on reproducing the issue.
I think this boolean should just go and have the queue logic always on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorban I think it needs to be restored, so it will be easier to performance test the effect of recycling channels.
For a benchmark we can subclass HTTP2ServerConnectionFactory.newConnection() and explicitly call the boolean setter on the HTTP2Connection.

{
if (isRecycleHttpChannels())
{
try (AutoLock l = lock.lock())
{
return channels.poll();
}
}
else
{
return null;
}
this.recycleHttpChannels = recycleHttpChannels;
}
*/

public boolean upgrade(Request request, HttpFields.Mutable responseFields)
{
Expand Down
1 change: 1 addition & 0 deletions ...tp2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ public void succeeded()
}
}
_httpChannel.recycle();
_connection.offerHttpChannel(_httpChannel);
}

@Override
Expand Down
Loading