This is an automated email from the ASF dual-hosted git repository. remm pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push: new 33eccc7 Simplify regular endpoint writes by removing write(Non)BlockingDirect 33eccc7 is described below commit 33eccc77048bd15131f8f9f5d95720399719952a Author: remm <r...@apache.org> AuthorDate: Mon Nov 25 17:44:00 2019 +0100 Simplify regular endpoint writes by removing write(Non)BlockingDirect The performance difference is null on what I exercised (static files). However, this removes code and makes buffering behavior consistent, avoiding a possible issue mentioned in the fixme (small writes causing a performance degradation). Following this I don't find any additional interesting cleanup. After testing to verify things are not broken, I will work on backporting to 8.5 piece by piece. --- java/org/apache/tomcat/util/net/AprEndpoint.java | 51 ------------ .../apache/tomcat/util/net/SocketWrapperBase.java | 96 ++++------------------ webapps/docs/changelog.xml | 5 ++ 3 files changed, 22 insertions(+), 130 deletions(-) diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java index 10322da..ebfd51b 100644 --- a/java/org/apache/tomcat/util/net/AprEndpoint.java +++ b/java/org/apache/tomcat/util/net/AprEndpoint.java @@ -2250,57 +2250,6 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB @Override - protected void writeBlockingDirect(ByteBuffer from) throws IOException { - if (from.isDirect()) { - super.writeBlockingDirect(from); - } else { - // The socket write buffer capacity is socket.appWriteBufSize - ByteBuffer writeBuffer = socketBufferHandler.getWriteBuffer(); - int limit = writeBuffer.capacity(); - while (from.remaining() >= limit) { - socketBufferHandler.configureWriteBufferForWrite(); - transfer(from, writeBuffer); - doWrite(true); - } - - if (from.remaining() > 0) { - socketBufferHandler.configureWriteBufferForWrite(); - transfer(from, writeBuffer); - } - } - } - - - @Override - protected void writeNonBlockingDirect(ByteBuffer from) throws IOException { - if (from.isDirect()) { - super.writeNonBlockingDirect(from); - } else { - // The socket write buffer capacity is socket.appWriteBufSize - ByteBuffer writeBuffer = socketBufferHandler.getWriteBuffer(); - int limit = writeBuffer.capacity(); - while (from.remaining() >= limit) { - socketBufferHandler.configureWriteBufferForWrite(); - transfer(from, writeBuffer); - int newPosition = writeBuffer.position() + limit; - doWrite(false); - if (writeBuffer.position() != newPosition) { - // Didn't write the whole amount of data in the last - // non-blocking write. - // Exit the loop. - return; - } - } - - if (from.remaining() > 0) { - socketBufferHandler.configureWriteBufferForWrite(); - transfer(from, writeBuffer); - } - } - } - - - @Override protected void doWrite(boolean block, ByteBuffer from) throws IOException { Lock readLock = getBlockingStatusReadLock(); WriteLock writeLock = getBlockingStatusWriteLock(); diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java index e81eb26..777276d 100644 --- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java +++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java @@ -531,7 +531,7 @@ public abstract class SocketWrapperBase<E> { protected void writeBlocking(byte[] buf, int off, int len) throws IOException { socketBufferHandler.configureWriteBufferForWrite(); int thisTime = transfer(buf, off, len, socketBufferHandler.getWriteBuffer()); - while (socketBufferHandler.getWriteBuffer().remaining() == 0) { + while (!socketBufferHandler.getWriteBuffer().hasRemaining()) { len = len - thisTime; off = off + thisTime; doWrite(true); @@ -555,46 +555,10 @@ public abstract class SocketWrapperBase<E> { * @throws IOException If an IO error occurs during the write */ protected void writeBlocking(ByteBuffer from) throws IOException { - if (socketBufferHandler.isWriteBufferEmpty()) { - // Socket write buffer is empty. Write the provided buffer directly - // to the network. - // TODO Shouldn't smaller writes be buffered anyway? - writeBlockingDirect(from); - } else { - // Socket write buffer has some data. - socketBufferHandler.configureWriteBufferForWrite(); - // Put as much data as possible into the write buffer - transfer(from, socketBufferHandler.getWriteBuffer()); - // If the buffer is now full, write it to the network and then write - // the remaining data directly to the network. - if (!socketBufferHandler.isWriteBufferWritable()) { - doWrite(true); - writeBlockingDirect(from); - } - } - } - - - /** - * Writes directly to the network, bypassing the socket write buffer. - * - * @param from The ByteBuffer containing the data to be written - * - * @throws IOException If an IO error occurs during the write - */ - protected void writeBlockingDirect(ByteBuffer from) throws IOException { - // The socket write buffer capacity is socket.appWriteBufSize - // TODO This only matters when using TLS. For non-TLS connections it - // should be possible to write the ByteBuffer in a single write - int limit = socketBufferHandler.getWriteBuffer().capacity(); - int fromLimit = from.limit(); - while (from.remaining() >= limit) { - from.limit(from.position() + limit); - doWrite(true, from); - from.limit(fromLimit); - } - - if (from.remaining() > 0) { + socketBufferHandler.configureWriteBufferForWrite(); + transfer(from, socketBufferHandler.getWriteBuffer()); + while (from.hasRemaining()) { + doWrite(true); socketBufferHandler.configureWriteBufferForWrite(); transfer(from, socketBufferHandler.getWriteBuffer()); } @@ -664,11 +628,12 @@ public abstract class SocketWrapperBase<E> { protected void writeNonBlocking(ByteBuffer from) throws IOException { - if (nonBlockingWriteBuffer.isEmpty() && socketBufferHandler.isWriteBufferWritable()) { + if (from.hasRemaining() && nonBlockingWriteBuffer.isEmpty() + && socketBufferHandler.isWriteBufferWritable()) { writeNonBlockingInternal(from); } - if (from.remaining() > 0) { + if (from.hasRemaining()) { // Remaining data must be buffered nonBlockingWriteBuffer.add(from); } @@ -684,44 +649,17 @@ public abstract class SocketWrapperBase<E> { * @throws IOException If an IO error occurs during the write */ protected void writeNonBlockingInternal(ByteBuffer from) throws IOException { - if (socketBufferHandler.isWriteBufferEmpty()) { - writeNonBlockingDirect(from); - } else { - socketBufferHandler.configureWriteBufferForWrite(); - transfer(from, socketBufferHandler.getWriteBuffer()); - if (!socketBufferHandler.isWriteBufferWritable()) { - doWrite(false); - if (socketBufferHandler.isWriteBufferWritable()) { - writeNonBlockingDirect(from); - } - } - } - } - - - protected void writeNonBlockingDirect(ByteBuffer from) throws IOException { - // The socket write buffer capacity is socket.appWriteBufSize - // TODO This only matters when using TLS. For non-TLS connections it - // should be possible to write the ByteBuffer in a single write - int limit = socketBufferHandler.getWriteBuffer().capacity(); - int fromLimit = from.limit(); - while (from.remaining() >= limit) { - int newLimit = from.position() + limit; - from.limit(newLimit); - doWrite(false, from); - from.limit(fromLimit); - if (from.position() != newLimit) { - // Didn't write the whole amount of data in the last - // non-blocking write. - // Exit the loop. - return; + socketBufferHandler.configureWriteBufferForWrite(); + transfer(from, socketBufferHandler.getWriteBuffer()); + while (from.hasRemaining() && !socketBufferHandler.isWriteBufferWritable()) { + doWrite(false); + if (socketBufferHandler.isWriteBufferWritable()) { + socketBufferHandler.configureWriteBufferForWrite(); + transfer(from, socketBufferHandler.getWriteBuffer()); + } else { + break; } } - - if (from.remaining() > 0) { - socketBufferHandler.configureWriteBufferForWrite(); - transfer(from, socketBufferHandler.getWriteBuffer()); - } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 486fe95..38bdc88 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -59,6 +59,11 @@ <bug>63949</bug>: Fix non blocking write problems with NIO due to the need for a write loop. (remm) </fix> + <fix> + Simplify regular endpoint writes by removing write(Non)BlockingDirect. + All regular writes will now be buffered for a more predictable + behavior. (remm) + </fix> </changelog> </subsection> <subsection name="Web applications"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org