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

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[michaelandrepearce]

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)


---

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

[franz1981]

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?


---

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

[franz1981]

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!


---

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

[michaelandrepearce]

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.


---

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

[michaelandrepearce]

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.


---

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

[franz1981]

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...


---

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

[franz1981]

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.


---

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

[michaelandrepearce]

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. 


---

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

[michaelandrepearce]

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.


---

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

[michaelandrepearce]

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


---

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

[michaelandrepearce]

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


---

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

[michaelandrepearce]

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.


---

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

[franz1981]

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 
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




---