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/13 22:22:10 UTC
[GitHub] activemq-artemis pull request #1777: ARTEMIS-1606 - Change AddressInfo Routi...
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/1777
ARTEMIS-1606 - Change AddressInfo RoutingType Set to use EnumSet
Change all use from Set<RoutingType> to EnumSet<RoutingType>
Deprecating any old exposed interfaces but keeping for back compatibility.
Address info to avoid iterator on getRoutingType hotpath, like wise can be avoided where single RoutingType is passed in.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1606
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1777.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 #1777
----
commit 01a546de10f1fd1315d9c47488f9a1dfd3e64a47
Author: Michael André Pearce <mi...@...>
Date: 2018-01-13T19:47:58Z
ARTEMIS-1606 - Change AddressInfo RoutingType Set to use EnumSet
Change all use from Set<RoutingType> to EnumSet<RoutingType>
Deprecating any old exposed interfaces but keeping for back compatibility.
Address info to avoid iterator on getRoutingType hotpath, like wise can be avoided where single RoutingType is passed in.
----
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
@michaelandrepearce
Looking at the changes via Github it seems clean and effective and I admit I'm a fan of using the "right" collection when needed, but I want to use some time tomorrow to check the code locally too and run some CI tests on it: similar to `SimpleString` changes I prefer to be 100% that isn't impacting negatively with hidden behaviours
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
@franz1981 i was waiting on you, to give a thumbs up before i merged, is that a thumbs up?
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
Sure, using JMH to measure performance of the two AddressInfo Implementations.
Benchmark Mode Cnt Score Error Units
MyBenchmark.testEnumSetAddressInfo thrpt 200 198607467.630 ± 507344.268 ops/s
MyBenchmark.testHashSetAddressInfo thrpt 200 22849376.205 ± 216480.582 ops/s
This is simulating what happens in the hotpath of sending a message in doSend within ServerSessionImpl. Which is creating an AddressInfo and then getRoutingType.
As you note the throughput is an order of magnitude of 8x.
Also from a java memory footprint this is far better.
AddressInfo using HashSet with one routing type -> 208 bytes
AddressInfo using HashSet with two routing types -> 240 bytes
AddressInfo using EnumSet with one routing type -> 72 bytes
AddressInfo using EnumSet with two routing types -> 72 bytes
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
@michaelandrepearce I'm looking at it locally right now :+1:
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
@michaelandrepearce I'm running a test before and after, but first can you try add ".jvmArgs("-XX:+UseG1GC")" and `.forks(2)` to your benchmark?
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
These are the flamegraph of a couple of profiled tests, one on master:
Where the violet bars are the `AddressInfo::new` and the `AddressInfo::getRoutingType` (+ iterator) while with this PR:
There isn't anymore any cost associated with `AddressInfo`: for me is a thumbs up!
Althouh it seems negligible (at a first look) the impact of this change has changed the garbage produced and the CPU time required to call `doSend`: indeed the latencies are better too.
Well done :100:
---
[GitHub] activemq-artemis pull request #1777: ARTEMIS-1606 - Change AddressInfo Routi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1777
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
@franz1981 you happy for this to merge?
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
@franz1981 ping.
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
Thanks, @michaelandrepearce. Nice results!
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
Very good results! A lil OT but seems that we really start need a JMH folder with all the benchs on Artemis eh? :P
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
This looks good to me too
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
Great thanks, ill merge then.
---
[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1777
Any metrics to quantify the benefit and justify this change?
---