Skip to content

Make HttpOutput.println() simpler and faster #12530

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

Merged
merged 4 commits into from
Mar 20, 2025
Merged

Make HttpOutput.println() simpler and faster #12530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62fc0bf
fix tests
lorban Mar 18, 2025
5f943dc
faster and simpler HttpOutput.println
lorban Mar 18, 2025
6444495
fix checkstyle
lorban Mar 18, 2025
dbc57d2
proper fallback to blocking mode when cannot aggregate
lorban Mar 18, 2025
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
61 changes: 33 additions & 28 deletions ...-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/HttpOutputTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.emptyString;
import static org.hamcrest.Matchers.endsWith;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

Expand Down Expand Up @@ -149,7 +151,7 @@ public void testSendInputStreamBig() throws Exception
_contentServlet._contentInputStream = IOResources.asInputStream(big);
String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand Down Expand Up @@ -203,7 +205,7 @@ public void testSendChannelBig() throws Exception
_contentServlet._contentChannel = Files.newByteChannel(big.getPath());
String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand All @@ -227,7 +229,7 @@ public void testSendResourceBig() throws Exception
_contentServlet._contentResource = big;
String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand Down Expand Up @@ -315,7 +317,7 @@ public void testWriteByte() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand All @@ -330,7 +332,7 @@ public void testWriteSmall() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand All @@ -345,7 +347,7 @@ public void testWriteMed() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand All @@ -360,7 +362,7 @@ public void testWriteLarge() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
}

Expand Down Expand Up @@ -458,7 +460,7 @@ public void testWriteBufferSmall() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -474,7 +476,7 @@ public void testWriteBufferMed() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -490,7 +492,7 @@ public void testWriteBufferLarge() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand Down Expand Up @@ -555,7 +557,7 @@ public void testAsyncWriteByte() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -572,7 +574,7 @@ public void testAsyncWriteSmall() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -589,7 +591,7 @@ public void testAsyncWriteMed() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -606,7 +608,7 @@ public void testAsyncWriteLarge() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -627,7 +629,7 @@ public void testAsyncWriteHuge() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}

Expand All @@ -643,7 +645,7 @@ public void testAsyncWriteBufferSmall() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -660,7 +662,7 @@ public void testAsyncWriteBufferMed() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -677,7 +679,7 @@ public void testAsyncWriteBufferLarge() throws Exception

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -695,7 +697,7 @@ public void testAsyncWriteBufferLargeDirect()

String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, endsWith(toUTF8String(big)));
assertThat(_contentServlet._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false));
}
Expand All @@ -714,9 +716,9 @@ public void testAsyncWriteBufferLargeHEAD() throws Exception
String response = _connector.getResponse("HEAD / HTTP/1.0\nHost: localhost:80\n\n");
assertThat(_contentServlet._owp.get() - start, Matchers.greaterThan(0));
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, Matchers.not(containsString("Content-Length")));
assertThat(response, Matchers.not(containsString("1\tThis is a big file")));
assertThat(response, Matchers.not(containsString("400\tThis is a big file")));
assertThat(response, not(containsString("Content-Length")));
assertThat(response, not(containsString("1\tThis is a big file")));
assertThat(response, not(containsString("400\tThis is a big file")));
}

@Test
Expand Down Expand Up @@ -753,7 +755,7 @@ public void testAsyncWriteSimpleKnownHEAD() throws Exception
assertThat(_contentServlet._owp.get() - start, Matchers.equalTo(1));
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, containsString("Content-Length: 11"));
assertThat(response, Matchers.not(containsString("simple text")));
assertThat(response, not(containsString("simple text")));
}

@Test
Expand Down Expand Up @@ -1004,13 +1006,13 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
public void testPrint() throws Exception
{
ByteArrayOutputStream bout = new ByteArrayOutputStream();
PrintWriter exp = new PrintWriter(bout);
PrintWriter exp = new PrintWriter(bout, true, StandardCharsets.UTF_8);
HttpServlet servlet = new HttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
{
response.setCharacterEncoding("UTF8");
response.setCharacterEncoding("UTF-8");
HttpOutput out = (HttpOutput)response.getOutputStream();

// @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck
Expand Down Expand Up @@ -1048,9 +1050,12 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
};
_servletContextHandler.addServlet(servlet, "/test");
_server.start();
String response = _connector.getResponse("GET /test HTTP/1.0\nHost: localhost:80\n\n");
ByteBuffer responseBuf = _connector.getResponse(ByteBuffer.wrap("GET /test HTTP/1.0\nHost: localhost:80\n\n".getBytes(StandardCharsets.UTF_8)));
String response = BufferUtil.toString(responseBuf, StandardCharsets.UTF_8);
assertThat(response, containsString("HTTP/1.1 200 OK"));
assertThat(response, containsString(bout.toString()));
String expected = bout.toString(StandardCharsets.UTF_8);
assertThat(expected, not(emptyString()));
assertThat(response.replace("\r\n", System.lineSeparator()), containsString(expected));
}

@Test
Expand Down
98 changes: 36 additions & 62 deletions jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,8 @@
import java.io.IOException;
import java.io.InputStream;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.channels.ReadableByteChannel;
import java.nio.channels.WritePendingException;
import java.nio.charset.Charset;
import java.nio.charset.CharsetEncoder;
import java.nio.charset.CoderResult;
import java.nio.charset.CodingErrorAction;
import java.util.concurrent.CancellationException;

import jakarta.servlet.RequestDispatcher;
Expand All @@ -44,7 +39,6 @@
import org.eclipse.jetty.util.IteratingCallback;
import org.eclipse.jetty.util.NanoTime;
import org.eclipse.jetty.util.thread.AutoLock;
import org.eclipse.jetty.util.thread.ThreadIdPool;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -123,7 +117,6 @@ enum ApiState
}

private static final Logger LOG = LoggerFactory.getLogger(HttpOutput.class);
private static final ThreadIdPool<CharsetEncoder> _encoder = new ThreadIdPool<>();

private final ServletChannel _servletChannel;
private final ServletChannelState _channelState;
Expand Down Expand Up @@ -1120,73 +1113,54 @@ private void print(String s, boolean eoln) throws IOException
throw new IOException("Closed");

s = String.valueOf(s);

String charset = _servletChannel.getServletContextResponse().getCharacterEncoding(false);
CharsetEncoder encoder = _encoder.take();
if (encoder == null || !encoder.charset().name().equalsIgnoreCase(charset))
// String.getBytes() is much faster than a CharsetEncoder.encode() loop that would also need to estimate
// the destination buffer size, so this compensates for the rare extra copy that needs to be done when
// the aggregation buffer is full.
byte[] bytes = s.getBytes(charset);
if (!eoln)
{
encoder = Charset.forName(charset).newEncoder();
encoder.onMalformedInput(CodingErrorAction.REPLACE);
encoder.onUnmappableCharacter(CodingErrorAction.REPLACE);
write(bytes);
}
ByteBufferPool pool = _servletChannel.getRequest().getComponents().getByteBufferPool();
RetainableByteBuffer out = pool.acquire((int)(1 + (s.length() + 2) * encoder.averageBytesPerChar()), false);
try
else
{
CharBuffer in = CharBuffer.wrap(s);
CharBuffer crlf = eoln ? CharBuffer.wrap("\r\n") : null;
ByteBuffer byteBuffer = out.getByteBuffer();
BufferUtil.flipToFill(byteBuffer);
int len = bytes.length + IO.CRLF_BYTES.length;

while (true)
// If there is enough room left in the aggregation buffer, just fill it;
// otherwise either do 2 writes if blocking or copy into a bigger byte array if async.
boolean aggregated = false;
boolean blocking = false;
try (AutoLock ignored = _channelState.lock())
{
CoderResult result;
if (in.hasRemaining())
{
result = encoder.encode(in, byteBuffer, crlf == null);
if (result.isUnderflow())
if (crlf == null)
break;
else
continue;
}
else if (crlf != null && crlf.hasRemaining())
if (len <= _bufferSize)
{
result = encoder.encode(crlf, byteBuffer, true);
if (result.isUnderflow())
lockedAcquireBuffer();
if (len <= maximizeAggregateSpace())
{
if (!encoder.flush(byteBuffer).isUnderflow())
result.throwException();
break;
BufferUtil.fill(_aggregate.getByteBuffer(), bytes, 0, bytes.length);
aggregated = true;
}
}
else
break;

if (result.isOverflow())
{
BufferUtil.flipToFlush(byteBuffer, 0);
RetainableByteBuffer bigger = pool.acquire(out.capacity() + s.length() + 2, out.isDirect());
BufferUtil.flipToFill(bigger.getByteBuffer());
bigger.getByteBuffer().put(byteBuffer);
out.release();
BufferUtil.flipToFill(bigger.getByteBuffer());
out = bigger;
byteBuffer = bigger.getByteBuffer();
continue;
}

result.throwException();
if (!aggregated && _apiState == ApiState.BLOCKING)
blocking = true;
}

BufferUtil.flipToFlush(byteBuffer, 0);
write(byteBuffer.array(), byteBuffer.arrayOffset(), byteBuffer.remaining());
}
finally
{
out.release();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the above write() call is async, out.release() may be called before the write is done.

encoder.reset();
_encoder.offer(encoder);
if (aggregated)
{
write(IO.CRLF_BYTES);
}
else if (blocking)
{
write(bytes);
write(IO.CRLF_BYTES);
}
else
{
byte[] bytesWithCrLf = new byte[len];
System.arraycopy(bytes, 0, bytesWithCrLf, 0, bytes.length);
System.arraycopy(IO.CRLF_BYTES, 0, bytesWithCrLf, bytes.length, IO.CRLF_BYTES.length);
write(bytesWithCrLf);
}
}
}

Expand Down
Loading