[GitHub] activemq-artemis pull request #2449: ARTEMIS-2191 Makes PostOfficeImpl::rout...

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2449: ARTEMIS-2191 Makes PostOfficeImpl::rout...

rstancel
GitHub user franz1981 opened a pull request:

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

     ARTEMIS-2191 Makes PostOfficeImpl::route common paths inlineable

    It allows PostOfficeImpl::route to have a better inlining while dealing with common paths that require to be fast.
    It includes some optimisation to reduce the allocations on the hot paths.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/franz1981/activemq-artemis optimized_route

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2449.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 #2449
   
----
commit ebdb9b2f193fb3be641e996f9214b1adf29560df
Author: Francesco Nigro <nigro.fra@...>
Date:   2018-12-04T13:14:52Z

    ARTEMIS-2191 Makes PostOfficeImpl::route common paths inlineable
   
    Optimized route

commit 47cbc489f3c2ce9381994b72bd4fa43bf7c1d81f
Author: Francesco Nigro <nigro.fra@...>
Date:   2018-12-04T15:04:53Z

    ARTEMIS-2191 Makes PostOfficeImpl::route common paths inlineable
   
    Optimized processRoute

----


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    I will provide soon some number to justify these changes from the perf pov: from code quality pov it is a refactoring to make the code simpler to read in methods that are supposed to be called very often.
    The 2 commits are be left there just to simplify the code review: I will squash them into one if it will be approved.
    @michaelandrepearce @clebertsuconic Feel free to review it :+1:


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @franz1981 can you hold this?
   
    or better.. can you send these optimizations into my new-amqp branch?
   
   
    I have optimized routing to cache it.. and it would take me longer to refactor your changes in than yourself doing it.


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @clebertsuconic No worries I can just wait to review the PR that you will send too, so we keep separate the changes.
    Or you are curious to see if it helps your part too?


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    I have verified that the code isn't changing the current behaviour: the only failures I see are the same intermittent ones we have on CI (on master).


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @franz1981 close this please? it will cause me a lot of headache if someone merged it before my branch?
   
    I'm pulling your change with my PR.


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

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

    https://github.com/apache/activemq-artemis/pull/2449
 
    @franz1981, I'd still love to see numbers whenever you get the chance to post them. :+1:


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @franz1981 your changes (on my branch) are causing a few failures.
   
    I really like your optimizations here. but can you close this. .and resend once I'm done?


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @clebertsuconic I have tun the CI and it seems to work has expected (no additional failures), it's strange :O


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

[GitHub] activemq-artemis pull request #2449: ARTEMIS-2191 Makes PostOfficeImpl::rout...

rstancel
In reply to this post by rstancel
Github user franz1981 closed the pull request at:

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


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @jbertram TBH I haven't had the chance to measure it as it deserves, but I have found that it produces much less garbage (several MB/sec). I hope to find some time to do a fair comparison with master


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    @franz1981 the failures I mentioned are on my branch.
   
   
    I am asking you to defer your PR until after I merge mine on AMQP performance.
   
    This route change I'm making is a bit more functional than yours. It seems yours is a refactoring while mine had functionality involved.. hence I'm asking you to defer it
   
   
    Adding your change in top of mine added failues (the one you sent).


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

[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...

rstancel
In reply to this post by rstancel
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2449
 
    > It seems yours is a refactoring while mine had functionality involved.. hence I'm asking you to defer it
   
    Yep, got it :+1:
   
    > Adding your change in top of mine added failues (the one you sent).
   
    I see 2 reasons for that:
   
    - the context::clear that I have added is wrong
    - there are other cases where we need to clear the context that are not correct handled (due 2 my changes!)
   
    -


---