You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2018/01/08 19:19:09 UTC
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/1757
ARTEMIS-1586 Reduce GC pressure due to String allocations on Core protocol
@franz1981 i think this is cleaner merge of the two.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1586-FRANZ
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1757.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 #1757
----
commit 4ab1fea88610e46381141223479aec97e7fa04ac
Author: Francesco Nigro <ni...@...>
Date: 2018-01-04T14:22:05Z
ARTEMIS-1586 Reduce GC pressure due to String allocations on Core protocol
The commit contains:
- a general purpose interner implementation
- StringValue/SimpleString internrs specializations
- TypedProperties keys/values string interning for SessionSendMessage decoding
commit 794e56da99456e891284709b790bf8144d65b0f9
Author: Michael André Pearce <mi...@...>
Date: 2018-01-08T18:45:18Z
ARTEMIS-1586 Refactor to make more generic
* Move byte util code into ByteUtil
* Re-use the new equals method in SimpleString
* Apply same pools/interners to client decode
* Create String to SimpleString pools/interners for property access via String keys (producer and consumer benefits)
* Lazy init the pools on withing the get methods of CoreMessageObjectPools to get the specific pool, to avoid having this scattered every where.
----
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 once the build of that PR for fixing the leaking RemotingConnectionImpl is green, ill merge it, and then ill rebase this branch again.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 so i was looking at this, agree things look better, one thing ive noticed though is TypedProperties is being created server side due to routingType "ouch" this is done on every message, i was under the belief all the core stuff needed server side should be in the CoreMessage as top level, to avoid deserialising the typed properties. Seems also a couple of other core properties used server side have been squirrelled into a property. I guess at the time for ease of adding a feature.
Do we want to look address this also (NOT for this PR but something id like to discuss with you), as its not helping matters on the front of GC, as obviously avoidance is better?
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
Here is more evidence we really want bits like routing type not as a typed property but as a top level header, really dont want to be creating typed properties if we can not (it creates hashmap and everything ->
<img width="972" alt="screen shot 2018-01-13 at 18 02 09" src="https://user-images.githubusercontent.com/1387822/34908711-ee6ee63a-f88b-11e7-8b80-0bf26767ad80.png">
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160247350
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
I've implemented it in the last commit as:
```
@Override
public boolean equals(final Object other) {
if (this == other) {
return true;
}
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
if (s.hash != 0 && this.hash != 0) {
if (s.hash != this.hash)
return false;
}
if (s.str != null && this.str != null) {
return this.str.equals(s.str);
} else {
return ByteUtil.equals(this.data, s.data);
}
} else {
return false;
}
}
```
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 i stupidly didn't screenshot it, but yes there was slight improvement for the same test case, about a 0.5k uplift on average, 10.5 -> 11k for 2producers, 2consumers persistent AIO and without slave running, but master is configured for HA, i simply didn't start the slave as servers I'm using some reason could only get onto one of them.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
The RemotingConnectionImpl will hold reference to the packet decoder, until itself is GC'd. It will be long lived for bits like cluster connections etc on the server side, also it is held in maps like within RemotingServiceImpl.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
The results seems great: it is a huge drop on latencies!
So "Post-Change (2.5-SNAPSHOT local build - Using old 2.4 client)" is including the pooling too?
And most importantly, do you have the throughput measurements too?
>that was more about being kinder to users applications
:100:
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1757
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce
It is near to be fixed, but I've opened a separate JIRA for it:
Re memory leaks of `RemotingConnectionImpl` into `JaasCallbackHandler` probably @gtully or @jbertram would be super fast to indentify why are not collected propertly...
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce
I can only say that Christmas is far away, but this is looking just like a Christmas present to me eheh
What makes me (even) more happy is that without persistence there are more chances that the things will be faster too: can just say..well done Michael :+1:
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce Probably I've fixed it: it was subtle...
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160347432
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
And there is a correctness issue around it (in my code too!!!):
if `s` is `null` `s.data` will throw NPE!
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
Pre-Change
<img width="1422" alt="screen shot 2018-01-13 at 17 23 01" src="https://user-images.githubusercontent.com/1387822/34908377-877bb67e-f886-11e7-83cc-850b56167c7d.png">
Post-Change including the remove of the duplicate on decoding typedproperties (Using old 2.4 client)
<img width="1426" alt="screen shot 2018-01-13 at 17 22 17" src="https://user-images.githubusercontent.com/1387822/34908380-a1b4c0e4-f886-11e7-83de-ebad050b2bee.png">
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 re the checkProperties change, its not thread safe. And i'd rather not put synchronised block in.
Found this on running larger test suite. On checking duplicate it seems as efficient as it can be as in its just an object wrapping around the existing bytebuf, with new index's effectively, so i reverted that change (i get same perf result btw on load testing).
I think real solution is to get those core properties like routingtype to the core message level, as such then they will be more efficient and lazyproperties can be truely honoured server side and not decoded at all.
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160355213
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
I will perform some benchs to see if it will impact positively on checks and/or pattern matching based on equals: only in that case I will send a separate improvement PR :+1:
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 I've spotted something else the ActiveMQMessage wraps Strings into SimpleStrings instead of calling the core Message string equiv methods, as such its not getting the benefit, ill do this tonight/tomorrow (depending how long Cleberts web conf goes, btw you're attending right? )
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
Looking at the code it seems good to me: I've a couple of things to finish first but I will provide ASAP some measurements both server/client side of the impacts :)
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
What I've noticed doing some fast tests is that there is just one (the same) dead living (ie detroyed but not garbage collected) `RemoteCollectionImpl` that reference the pools after several tests: I need to undestand if we have to get rid of it in order to avoid memory leaks.
Re the hashCode with ByteBuf that I've implemented, seems with UUID that is not working propertly due to integer overflow: I will review/fix it soon
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 could you fully expand the fields in references pane until root? I suspect the login context is there holding the jaas callback hadler
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce
> What else you think we need do to merge all this? Be good to get it in, before we crack on with the next thing tbh.
I will love to provide some basic unit tests for the `*Pool`s implementation to strengthen it: anyway I can push the tests as a separate PR too...
> On checking duplicate it seems as efficient as it can be as in its just an object wrapping around the existing bytebuf, with new index's effectively, so i reverted that change
Oh, just reading the name `duplicate` I've thought Netty was performing a copy :(...BTW if we avoid that "sliced" allocation on an hot path I'm sure it will help GC when many producers/consumers will be involved, but if we will let it as it is I'm happy anyway considering that is not 100% the purpose of this PR
> (i get same perf result btw on load testing, its the pooling thats giving the perf boost)
Good to know, if it will turn to be effective as it seems I will provide something similar into `proton-j` too
> I think real solution is to get those core properties like routingtype to the core message level, as such then they will be more efficient and lazyproperties can be truely honoured server side and not decoded at all.
Agree, that's the real deal indeed
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160255842
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
I don't see the benefit of this change, for GC.
Why i moved the equals code between two byte arrays into ByteUtil and re-used the equals, was to keep the logic that is checked equal between the bytebuf equals and the equals of the String the same called code. Adding these extra checks, could allow for differences in result now, and for very marginal improvement at best IMO.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 just pushed to this brach that last change.
What else you think we need do to merge all this? Be good to get it in, before we crack on with the next thing tbh.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce @clebertsuconic
First benchs seems promising: I've used the Artemis master vs this patched version with 1 GB (-Xms -Xmx) of heap...
master version:
PR version:
322 vs 215 Minor GCs and about half of the time sent into GC.
I need to perform some checks on thoughput and the client, but I'm very pleased by the results, wdyt?
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce
> Do we want to look address this also (NOT for this PR but something id like to discuss with you), as its not helping matters on the front of GC, as obviously avoidance is better?
Sure! I think there is a great margin of improvement GC wise (and maybe latency/throughput too honestly) thanks to these changes, well done atfinding this!
> This seems a little heavy handed, when could just get current reader and writer index before decoding, then set them back after.
Looking at the current code I'm not seeing evident reasons why we can't avoid that duplication: it is a WIN WIN IMO! Probably this optimization could land into this same PR, but it is needed to change a lil' bit the PR description...
This entire PR looks to me as a whole great job of garbage reduction and I'm pretty about about it :+1:
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce I'm not sure I will be present honestly due to some backlog of work to finish first, I hope to finish soon and I will join for sure!
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160344540
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
It is just a further improvement, but I'm already happy with the code as it is now :+1:
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce Exactly...
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 which class? i don't find RemoteCollectionImpl
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
Uh typo error: I mean RemoteConnectionImpl :)
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160344336
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
The benefit is not from the GC pov but for normal `SimpleString` usage inside Map/Set: if the `SimpleString` has already hash computed or it is string-based it won't perform array comparison (optimzed ones for sure) but will benenfit of `String::equals` that is intrinsified by the JVM.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 i have now rebased again this brach, after merging https://github.com/apache/activemq-artemis/pull/1760 - if you wanted to check it out, and re-test.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 anything else left on this?
You couldn't possible provide an update as you did before with perf and gc stats? Also could you possible run before and after using the Core JMS wrapper.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
I will take a look on it now, but at a first look semms that there is a long living `RemoteConnectionImpl` that has a `destroyed` flag `true` which is not collected after a forced full GC
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
That's what I was talking about:
This `RemotingConnectionImpl` it has survived after several full GC and it is been already destroyed: it seems a leak to me, wdyt?
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
I can close the other one, but I'm just providing a couple of errors I've found too..I can do it here
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
I've provided the unit tests I was mentioning here: https://github.com/franz1981/activemq-artemis/tree/ARTEMIS-1586-FRANZ (the last commit)
This PR is a very good shape and I'm closing the other one: I'm running a complete CI on it but the perf results/memory footprint advantages seems very very good to me: @clebertsuconic wdyt?
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 will merge this, as seems no further feedback.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 i just merged your commit from that branch into this, so the tests now in.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
Just rebased branch ready for merge.
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 indeed it does looks like its improved things, obviously we have to note this is an artifical test case, but it does look good. Hats off to you, for thinking about using a "interner" pool :) +1
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@franz1981 done, also spotted a few wasteful bits in AMQP as well, where it went from String to SimpleString, back to String and back to SimpleString (or repeatedly created SimpleString from String), so cleaned that up .
---
[GitHub] activemq-artemis pull request #1757: ARTEMIS-1586 Reduce GC pressure due to ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1757#discussion_r160352626
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -259,22 +281,23 @@ public boolean equals(final Object other) {
if (other instanceof SimpleString) {
SimpleString s = (SimpleString) other;
- if (data.length != s.data.length) {
- return false;
- }
-
- for (int i = 0; i < data.length; i++) {
- if (data[i] != s.data[i]) {
- return false;
- }
- }
-
- return true;
+ return ByteUtil.equals(data, s.data);
--- End diff --
Cool. lets leave it then :)
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
Also another thing spotted, on decoding the typed properties a duplicate of the buffer is made
```
final ByteBuf byteBuf = buffer.duplicate().readerIndex(propertiesLocation);
properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
```
This seems a little heavy handed, when could just get current reader and writer index before decoding, then set them back after.
```
int readerIndex = buffer.readerIndex();
int writerIndex = buffer.writerIndex();
final ByteBuf byteBuf = buffer.readerIndex(propertiesLocation);
properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
buffer.setIndex(readerIndex, writerIndex);
```
WDYT?
---
[GitHub] activemq-artemis issue #1757: ARTEMIS-1586 Reduce GC pressure due to String ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1757
@michaelandrepearce Did you have had any chances to check if you've experienced similar improvements?
I hope to measure the client side too
---