[GitHub] activemq-artemis pull request #1777: ARTEMIS-1606 - Change AddressInfo Routi...

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

[GitHub] activemq-artemis pull request #1777: ARTEMIS-1606 - Change AddressInfo Routi...

RaiSaurabh
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 <michael.andre.pearce@...>
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.

----


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1777
 
    @franz1981 ping.


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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



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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

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

    https://github.com/apache/activemq-artemis/pull/1777
 
    @franz1981 you happy for this to merge?


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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:


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user cshannon commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1777
 
    This looks good to me too


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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?


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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



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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1777
 
    Thanks, @michaelandrepearce. Nice results!


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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?


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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?


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

RaiSaurabh
In reply to this post by RaiSaurabh
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:
    ![image](https://user-images.githubusercontent.com/13125299/35093335-7394c032-fc42-11e7-862e-936863f37eff.png)
    Where the violet bars are the `AddressInfo::new` and the `AddressInfo::getRoutingType` (+ iterator) while with this PR:
    ![image](https://user-images.githubusercontent.com/13125299/35093498-d48ef4ca-fc42-11e7-81c9-04565ea66c64.png)
    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:


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

[GitHub] activemq-artemis issue #1777: ARTEMIS-1606 - Change AddressInfo RoutingType ...

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

    https://github.com/apache/activemq-artemis/pull/1777
 
    Great thanks, ill merge then.


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

[GitHub] activemq-artemis pull request #1777: ARTEMIS-1606 - Change AddressInfo Routi...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user asfgit closed the pull request at:

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


---