You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/02/16 10:08:34 UTC
[GitHub] [activemq-artemis] sebthom opened a new pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
sebthom opened a new pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455
…Acceptor#getProtocols(Map)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576703957
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
I would actually prefer an early exit using `if(protocolManager == null) return "";` but that would result in more lines being touched. E.g.
```java
private static String getProtocols(final Map<String, ProtocolManager> protocolManager) {
if (protocolManager == null)
return "";
final StringBuilder sb = new StringBuilder();
final Set<String> strings = protocolManager.keySet();
for (final String string : strings) {
if (sb.length() > 0) {
sb.append(",");
}
sb.append(string);
}
return sb.toString();
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576711473
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
@franz1981 I just came across it while trying to understand/analyze the whole class to better understand what may cause https://issues.apache.org/jira/browse/ARTEMIS-3117. Maybe you can have a look there too. :-D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#issuecomment-779917654
@gemmellr Based on your feedback I cleanedup/simplified the method further.
I have no problem in opening a corresponding JIRA if required.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576706173
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
it's fine as long is more readable...
> but code like this just hurts my eyes.
Agree, but I would focus on things that hurt/benefit common usage, not just our eyes :P
Although I agree is nice to clean up stuff, to make it more readable :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom edited a comment on pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom edited a comment on pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#issuecomment-779917654
@gemmellr Based on your feedback I cleaned up/simplified the method further. I think now it should be pretty clear what the method actually does.
I have no problem in opening a corresponding JIRA if required.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576860189
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,15 +863,16 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
+ if (protocolManager == null || protocolManager.isEmpty())
+ return "";
+
StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
- Set<String> strings = protocolManager.keySet();
- for (String string : strings) {
- if (sb.length() > 0) {
- sb.append(",");
- }
- sb.append(string);
+ Set<String> strings = protocolManager.keySet();
+ for (String string : strings) {
Review comment:
Again if the aim is readability, using protocolManager(s).forEach(..) would seem nicer than seperate .keySet() and for-each loop, and perhaps also more efficient depending on impl.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,15 +863,16 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
+ if (protocolManager == null || protocolManager.isEmpty())
Review comment:
If we are aiding readability here, what about making the variable name "protocolManagers" since it is a map of them, not a single instance.
If statements are nicer with braces I find, less screwups later.
I have no idea where this is called from but I'm also initially surprised it doesnt just throw if the map is null/empty. When is it expected/valid to have no protocols?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom edited a comment on pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom edited a comment on pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#issuecomment-779917654
@gemmellr Based on your feedback I cleaned up/simplified the method further.
I have no problem in opening a corresponding JIRA if required.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelandrepearce commented on pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#issuecomment-779755718
This isnt NO Jira work. Can a JIRA item be raised
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3455: ARTEMIS-3121 Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#issuecomment-779848243
> This isnt NO Jira work. Can a JIRA item be raised
@michaelandrepearce this is just code cleanup. Nothing that would bring any value to a release notes...
The commit itself would be enough record of the change here.
If someone is creating a JIRA, it would be a task, as the is not an improvement, not a feature, not a bug fx.. no value on the release notes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576705908
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
For me it is also about reducing the cognitive load when reading source code. Now you know directly when a null was specified you get an empty string.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576703149
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
I don't know, but code like this just hurts my eyes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576703957
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
I would actually prefer an early exit using `if(protocolManager == null) return "";` but that would result in more lines being touched.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576703957
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
I would actually prefer an early exit using `if(protocolManager == null) return "";` but that would result in more lines being touched. E.g.
```java
private static String getProtocols(final Map<String, ProtocolManager> protocolManager) {
if (protocolManager == null)
return "";
final StringBuilder sb = new StringBuilder();
final Set<String> strings = protocolManager.keySet();
for (final String string : strings) {
if (sb.length() > 0) {
sb.append(",");
}
sb.append(string);
}
return sb.toString();
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576702323
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
how many times this is supposed to happen?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576713618
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
taking a look although i suggest to run async-profiler on it, disabling Dyantrace; that would help finding native hotspots too (if SSL is involved on Netty side).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576703957
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,17 +863,19 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
}
private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
- StringBuilder sb = new StringBuilder();
- if (protocolManager != null) {
+ if (protocolManager == null) {
Review comment:
I would actually prefer a early exit using `if(protocolManager == null) return "";` but that would result in more lines being touched.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org