On Tue, Mar 17, 2020 at 6:06 PM <[email protected]> wrote:
> 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 404a3a6 Remove sync
> 404a3a6 is described below
>
> commit 404a3a6366c0a8ba3e747db57e24dee271c44ac3
> Author: remm <[email protected]>
> AuthorDate: Tue Mar 17 18:06:23 2020 +0100
>
> Remove sync
>
> keyFor syncs on a global lock, so remove it when processing sockets.
>
Actually I misread the code and the lock is only tied to the channel and
not the selector, so it's very cheap. I'll amend the changelog. Let me know
if this breaks something and I'll revert this, it's clearly has zero
measureable impact at this point.
Rémy
> ---
> java/org/apache/tomcat/util/net/NioEndpoint.java | 50
> +++++++++++-------------
> webapps/docs/changelog.xml | 4 ++
> 2 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java
> b/java/org/apache/tomcat/util/net/NioEndpoint.java
> index f753b9a..7012782 100644
> --- a/java/org/apache/tomcat/util/net/NioEndpoint.java
> +++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
> @@ -1530,33 +1530,29 @@ public class NioEndpoint extends
> AbstractJsseEndpoint<NioChannel,SocketChannel>
> socketWrapper.close();
> return;
> }
> - SelectionKey key =
> socket.getIOChannel().keyFor(poller.getSelector());
>
> try {
> int handshake = -1;
> -
> try {
> - if (key != null) {
> - if (socket.isHandshakeComplete()) {
> - // No TLS handshaking required. Let the
> handler
> - // process this socket / event combination.
> - handshake = 0;
> - } else if (event == SocketEvent.STOP || event ==
> SocketEvent.DISCONNECT ||
> - event == SocketEvent.ERROR) {
> - // Unable to complete the TLS handshake.
> Treat it as
> - // if the handshake failed.
> - handshake = -1;
> - } else {
> - handshake =
> socket.handshake(key.isReadable(), key.isWritable());
> - // The handshake process reads/writes from/to
> the
> - // socket. status may therefore be OPEN_WRITE
> once
> - // the handshake completes. However, the
> handshake
> - // happens when the socket is opened so the
> status
> - // must always be OPEN_READ after it
> completes. It
> - // is OK to always set this as it is only
> used if
> - // the handshake completes.
> - event = SocketEvent.OPEN_READ;
> - }
> + if (socket.isHandshakeComplete()) {
> + // No TLS handshaking required. Let the handler
> + // process this socket / event combination.
> + handshake = 0;
> + } else if (event == SocketEvent.STOP || event ==
> SocketEvent.DISCONNECT ||
> + event == SocketEvent.ERROR) {
> + // Unable to complete the TLS handshake. Treat it
> as
> + // if the handshake failed.
> + handshake = -1;
> + } else {
> + handshake = socket.handshake(event ==
> SocketEvent.OPEN_READ, event == SocketEvent.OPEN_WRITE);
> + // The handshake process reads/writes from/to the
> + // socket. status may therefore be OPEN_WRITE once
> + // the handshake completes. However, the handshake
> + // happens when the socket is opened so the status
> + // must always be OPEN_READ after it completes. It
> + // is OK to always set this as it is only used if
> + // the handshake completes.
> + event = SocketEvent.OPEN_READ;
> }
> } catch (IOException x) {
> handshake = -1;
> @@ -1573,23 +1569,23 @@ public class NioEndpoint extends
> AbstractJsseEndpoint<NioChannel,SocketChannel>
> state = getHandler().process(socketWrapper,
> event);
> }
> if (state == SocketState.CLOSED) {
> - poller.cancelledKey(key, socketWrapper);
> +
> poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()),
> socketWrapper);
> }
> } else if (handshake == -1 ) {
> getHandler().process(socketWrapper,
> SocketEvent.CONNECT_FAIL);
> - poller.cancelledKey(key, socketWrapper);
> +
> poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()),
> socketWrapper);
> } else if (handshake == SelectionKey.OP_READ){
> socketWrapper.registerReadInterest();
> } else if (handshake == SelectionKey.OP_WRITE){
> socketWrapper.registerWriteInterest();
> }
> } catch (CancelledKeyException cx) {
> - poller.cancelledKey(key, socketWrapper);
> +
> poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()),
> socketWrapper);
> } catch (VirtualMachineError vme) {
> ExceptionUtils.handleThrowable(vme);
> } catch (Throwable t) {
> log.error(sm.getString("endpoint.processing.fail"), t);
> - poller.cancelledKey(key, socketWrapper);
> +
> poller.cancelledKey(socket.getIOChannel().keyFor(poller.getSelector()),
> socketWrapper);
> } finally {
> socketWrapper = null;
> event = null;
> diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> index 69ffa99..34d3cd6 100644
> --- a/webapps/docs/changelog.xml
> +++ b/webapps/docs/changelog.xml
> @@ -67,6 +67,10 @@
> for <code>URIEncoding</code> is a superset of US-ASCII as
> required by
> RFC7230. (markt)
> </add>
> + <fix>
> + Avoid always retrieving the NIO poller selection key when
> processing
> + to reduce sync. (remm)
> + </fix>
> </changelog>
> </subsection>
> </section>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
>
