[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
GitHub user franz1981 opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1849

    ARTEMIS-1656 OpenWire scalability improvements

    It includes:
    - direct transport buffer pooling
    - groupId SimpleString pooling
    - exclusive OpenWireFormat per session and connection (in/out) to avoid contention
    - cached trace check on JBoss server logger to avoid contention
    - changed lastSent volatile set into lazy set to avoid full barrier cost on x86
    - stateless OpenWireMessageConverter

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/franz1981/activemq-artemis open_wire_tests

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1849.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1849
   
----
commit 75564032def0c1aa79de824a51e53eab0c2a8791
Author: Francesco Nigro <nigro.fra@...>
Date:   2018-02-03T07:03:36Z

    ARTEMIS-1656 OpenWire scalability improvements
   
    It includes:
    - direct transport buffer pooling
    - groupId SimpleString pooling
    - exclusive OpenWireFormat per session and connection (in/out) to avoid contention
    - cached trace check on JBoss server logger to avoid contention
    - changed lastSent volatile set into lazy set to avoid full barrier cost on x86
    - stateless OpenWireMessageConverter

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    I'm running the some sanity tests on it.
    perf-wire now OpenWire is just 10/20% less than Core protocol for non durable messages (depending on message size), it has a much better scalability (like Core) and memory footprint thanks to transport buffer pooling.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165898852
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -713,12 +730,16 @@ public void reconnect(AMQConnectionContext existingContext, ConnectionInfo info)
           context.incRefCount();
        }
     
    +   private static void traceSendCommand(Command command) {
    +      ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +   }
    +
        /**
         * This will answer with commands to the client
         */
        public boolean sendCommand(final Command command) {
    -      if (ActiveMQServerLogger.LOGGER.isTraceEnabled()) {
    -         ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +      if (ACTIVE_MQ_SERVER_LOGGER_TRACE_ENABLED) {
    --- End diff --
   
    Was this really needed? From a code consistency POV, this is an odd one out, either all should follow in the code or not.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165898998
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -713,12 +730,16 @@ public void reconnect(AMQConnectionContext existingContext, ConnectionInfo info)
           context.incRefCount();
        }
     
    +   private static void traceSendCommand(Command command) {
    --- End diff --
   
    If you want a dedicated log message please add it to the Logger


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165899196
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -430,24 +442,29 @@ private void callFailureListeners(final ActiveMQException me) {
     
        // send a WireFormatInfo to the peer
        public void sendHandshake() {
    -      WireFormatInfo info = wireFormat.getPreferedWireFormatInfo();
    +      WireFormatInfo info = inWireFormat.getPreferedWireFormatInfo();
           sendCommand(info);
        }
     
        public ConnectionState getState() {
           return state;
        }
     
    +   private static void tracePhysicalSend(Connection transportConnection, Command command) {
    --- End diff --
   
    dedicated log message please add to the logger class


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165899293
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -248,8 +255,13 @@ private ConnectionInfo getConnectionInfo() {
     
        //tells the connection that
        //some bytes just sent
    -   public void bufferSent() {
    -      lastSent = System.currentTimeMillis();
    +   private void bufferSent() {
    +      //much cheaper than a volatile set if contended, but less precise (ie allows stale loads)
    +      LAST_SENT_UPDATER.lazySet(this, System.currentTimeMillis());
    +   }
    +
    +   private static void traceBufferReceived(Object connectionID, Command command) {
    --- End diff --
   
    dedicated log message method, please add to the logger.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165900265
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -430,24 +442,29 @@ private void callFailureListeners(final ActiveMQException me) {
     
        // send a WireFormatInfo to the peer
        public void sendHandshake() {
    -      WireFormatInfo info = wireFormat.getPreferedWireFormatInfo();
    +      WireFormatInfo info = inWireFormat.getPreferedWireFormatInfo();
           sendCommand(info);
        }
     
        public ConnectionState getState() {
           return state;
        }
     
    +   private static void tracePhysicalSend(Connection transportConnection, Command command) {
    +      logger.trace("connectionID: " + (transportConnection == null ? "" : transportConnection.getID()) + " SENDING: " + (command == null ? "NULL" : command));
    +   }
    +
        public void physicalSend(Command command) throws IOException {
     
           if (logger.isTraceEnabled()) {
    -         logger.trace("connectionID: " + (getTransportConnection() == null ? "" : getTransportConnection().getID())
    -                         + " SENDING: " + (command == null ? "NULL" : command));
    +         tracePhysicalSend(transportConnection, command);
           }
     
           try {
    -         ByteSequence bytes = wireFormat.marshal(command);
    -         ActiveMQBuffer buffer = OpenWireUtil.toActiveMQBuffer(bytes);
    +         final ByteSequence bytes = outWireFormat.marshal(command);
    +         final int bufferSize = bytes.length;
    +         final ActiveMQBuffer buffer = transportConnection.createTransportBuffer(bufferSize);
    --- End diff --
   
    should this be getTransportConnection, like the existing code below, or should the existing code below use the field directly also. Just a consistency POV.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165944067
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -248,8 +255,13 @@ private ConnectionInfo getConnectionInfo() {
     
        //tells the connection that
        //some bytes just sent
    -   public void bufferSent() {
    -      lastSent = System.currentTimeMillis();
    +   private void bufferSent() {
    +      //much cheaper than a volatile set if contended, but less precise (ie allows stale loads)
    +      LAST_SENT_UPDATER.lazySet(this, System.currentTimeMillis());
    +   }
    +
    +   private static void traceBufferReceived(Object connectionID, Command command) {
    --- End diff --
   
    I need to add a comment here: it is a trick to allow the caller to be inlined reducing bytecode size moving away uncommon paths...hence it doesn't have functional purposes.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165944739
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -713,12 +730,16 @@ public void reconnect(AMQConnectionContext existingContext, ConnectionInfo info)
           context.incRefCount();
        }
     
    +   private static void traceSendCommand(Command command) {
    +      ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +   }
    +
        /**
         * This will answer with commands to the client
         */
        public boolean sendCommand(final Command command) {
    -      if (ActiveMQServerLogger.LOGGER.isTraceEnabled()) {
    -         ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +      if (ACTIVE_MQ_SERVER_LOGGER_TRACE_ENABLED) {
    --- End diff --
   
    I know and it seems an issue with this logger, because it creates a point of contention between all the threads (all in the broker!) because it is locked (I will provide soon a trace of it): I don't think could exist a clean way to fix it other than on the jboss logger and it isn't feasible...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165979537
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -713,12 +730,16 @@ public void reconnect(AMQConnectionContext existingContext, ConnectionInfo info)
           context.incRefCount();
        }
     
    +   private static void traceSendCommand(Command command) {
    +      ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +   }
    +
        /**
         * This will answer with commands to the client
         */
        public boolean sendCommand(final Command command) {
    -      if (ActiveMQServerLogger.LOGGER.isTraceEnabled()) {
    -         ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +      if (ACTIVE_MQ_SERVER_LOGGER_TRACE_ENABLED) {
    --- End diff --
   
    Why not fix it at the route in jboss logger? Surely then it be global fix. Im much more happy to fix things at root source.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r165979719
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -248,8 +255,13 @@ private ConnectionInfo getConnectionInfo() {
     
        //tells the connection that
        //some bytes just sent
    -   public void bufferSent() {
    -      lastSent = System.currentTimeMillis();
    +   private void bufferSent() {
    +      //much cheaper than a volatile set if contended, but less precise (ie allows stale loads)
    +      LAST_SENT_UPDATER.lazySet(this, System.currentTimeMillis());
    +   }
    +
    +   private static void traceBufferReceived(Object connectionID, Command command) {
    --- End diff --
   
    Still same can be done by using the logger.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r166001057
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -713,12 +730,16 @@ public void reconnect(AMQConnectionContext existingContext, ConnectionInfo info)
           context.incRefCount();
        }
     
    +   private static void traceSendCommand(Command command) {
    +      ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +   }
    +
        /**
         * This will answer with commands to the client
         */
        public boolean sendCommand(final Command command) {
    -      if (ActiveMQServerLogger.LOGGER.isTraceEnabled()) {
    -         ActiveMQServerLogger.LOGGER.trace("sending " + command);
    +      if (ACTIVE_MQ_SERVER_LOGGER_TRACE_ENABLED) {
    --- End diff --
   
    You're quite right, probably I will revert it but it is actually an issue and probably could be turned into a lock-free way: actually it really create a point of contentions for any send of messages, affecting responses and consumers all together...not good!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r166001786
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -248,8 +255,13 @@ private ConnectionInfo getConnectionInfo() {
     
        //tells the connection that
        //some bytes just sent
    -   public void bufferSent() {
    -      lastSent = System.currentTimeMillis();
    +   private void bufferSent() {
    +      //much cheaper than a volatile set if contended, but less precise (ie allows stale loads)
    +      LAST_SENT_UPDATER.lazySet(this, System.currentTimeMillis());
    +   }
    +
    +   private static void traceBufferReceived(Object connectionID, Command command) {
    --- End diff --
   
    can't say if it has the same reason and could be addressed in that way: I do not need specific/common handling of those trace messages, but instead a way to packed them in separate methods leaving the callers smaller and able to be eligible for inlining.
    Making them more generic just for perf reasons could be even worst wdyt?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1849: ARTEMIS-1656 OpenWire scalability impro...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1849#discussion_r166011829
 
    --- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
    @@ -248,8 +255,13 @@ private ConnectionInfo getConnectionInfo() {
     
        //tells the connection that
        //some bytes just sent
    -   public void bufferSent() {
    -      lastSent = System.currentTimeMillis();
    +   private void bufferSent() {
    +      //much cheaper than a volatile set if contended, but less precise (ie allows stale loads)
    +      LAST_SENT_UPDATER.lazySet(this, System.currentTimeMillis());
    +   }
    +
    +   private static void traceBufferReceived(Object connectionID, Command command) {
    --- End diff --
   
    Its not generic, its still specific, its just following the conventions that specific logging lives in the Loggers, such as ActiveMQServerLogger (in this case maybe a new logger OpenWireLogger is needed/wanted)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    Btw I ran a perf test using my POC physical server setup, I see similar gains, so +1 from me (once comments are addressed)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    FYI gains were with non persistence, still need to sort native support of open wire format for persistent.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    @michaelandrepearce
    > FYI gains were with non persistence
   
    Yep, I need to specify it on the JIRA thanks!
    I'm fixing the last few bits and it is getting in the right shape to be merged :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    I'm going to merge this provided that is not causing any regressions on OpenWire tests/compatibility tests :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    I have no further comments. I haven’t had a chance to run test suites but as long as you run them and they pass I’m happy


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1849: ARTEMIS-1656 OpenWire scalability improvements

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1849
 
    @franz1981 can you rebase this please?


---
12