[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

asfgit
GitHub user clebertsuconic opened a pull request:

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

    ARTEMIS-1495 No more Executor.flush!

   

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

    $ git pull https://github.com/clebertsuconic/activemq-artemis artemis-1495

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

    https://github.com/apache/activemq-artemis/pull/1650.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 #1650
   
----
commit 0fced3de5525f583f10cd590ab59556cc3820453
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-02T09:51:43Z

    ARTEMIS-1495 Test simulating a dead lock on queue auto create under stress

commit 3f6e4c5afa7d2ff676805cbbda1aed92d27e0979
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-07T19:52:19Z

    ARTEMIS-1495 Removing flushes from codebase
   
    Instead of flushing we just need to make sure there are no more calls into
    page executors as we stop the PageManager.
   
    This will avoid any possible starvations or deadlocks here.

----


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

asfgit
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1650
 
    I've added a test for the `shutdownNode` method in a separate PR on your repository: let me know if it mirrors your expectation of what the method is supposed to do :+1:
   
    Re performance implications of this change: I've benchmarked it and the `synchronized` block makes a pure in-memory execution (i.e. no I/O involved) of the ProcessorBase about 7 time slower on my box when biased locking is disabled to mimic predicatable latencies and to avoid the JVM to trick us (`-XX:-UseBiasedLocking` is explained [here](https://mechanical-sympathy.blogspot.it/2011/11/biased-locking-osr-and-benchmarking-fun.html)).
    I have to do benchmarking on the overall broker too, but I suppose it will impact latencies if not the thoughput too.
   
    IMHO would be better to implement this using only (collaborative) lock-free algorithms that allow predicatable (and lower) latencies on hot paths: I've pushed another PR with an implementation of it for our use case (to review/discuss).
    On the hot paths it cost just a volatile load (on x86 is just a load and compare).


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    @franz1981 I'm surprised the lock would be that much slower.. there is nothing competing for that long..
    The advantage of the lock here is reentrancy. it's now possible to shutdown within the current executor. (even though I won't recommend that).
   
    I've tried a lock free thing using the state.. but it seemed simpler...
   
   
    I will try your PR.. just one thing.. you sent the PR to a wrong branch.. should be  clebertsuconic:artemis-1495


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    @clebertsuconic Argh!! Sorry about the  wrong PR: I will repush it!
    Just a couple of observations:
    to be 100% fair, the lock solution is not always that slow (like the in memory case is showing) when I/O (networking) is involved: in that case is just ~15% slower (eg that's on my notebook that has a single socket, with servers would be greater due to the context switch & kernel space journey for each task).
    > The advantage of the lock here is reentrancy.
   
    :100:  true! But I've avoided to implement it with a reentrant solution due to the advices received on a past PR: probably it is an antipattern (I suppose). If I'm wrong on that I can write a reentrant version too, but it will change further the logic (it is already complex!).
    The solution with the lock is much simpler hence I understand 100% if the lock free version wouldn't be accepted and if you'll find any error on the logic of what I've done we can fix it together (before any merge) :)
    Probably it could be made simpler too



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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    I have measured the lock version against the lock free and I don't see any difference.
   
    The code is supposedly single threaded most of the time.. for that reason using the JVM lock didn't change much the perf.
   
   
    I will take the lock free version  you made though.. as it's extending the same logic we have now.


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

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

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


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    I will close this as I am still working on this.


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

asfgit
In reply to this post by asfgit
GitHub user clebertsuconic reopened a pull request:

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

    ARTEMIS-1495 No more Executor.flush!

   

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

    $ git pull https://github.com/clebertsuconic/activemq-artemis artemis-1495

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

    https://github.com/apache/activemq-artemis/pull/1650.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 #1650
   
----
commit 331c0b063d70eb0dbdb74e27a82d5e11a6ee364b
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-02T09:51:43Z

    ARTEMIS-1495 Test simulating a dead lock on queue auto create under stress

commit 613e6e460866b267839d660275ea848d3fde870a
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-07T19:52:19Z

    ARTEMIS-1495 Removing flushes from codebase
   
    Instead of flushing we just need to make sure there are no more calls into
    page executors as we stop the PageManager.
   
    This will avoid any possible starvations or deadlocks here.

commit d5504a3f1f7269c270a164c1c1a38616b421dd50
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-08T09:05:35Z

    ARTEMIS-1495 Sanity tests for the ProcessorBase::shutdownNow feature

commit 4ae6bf958c6186ee8d35f1a76bff1087e98c8d92
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-08T11:03:49Z

    ARTEMIS-1495 Lock-free ProcessorBase::shutdownNow

commit fe4f45e69384e9fe9249e57f77ee477343ff1885
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-08T14:16:59Z

    ARTEMIS-1495 Fixing In Handler executor and added benchmark to measure impact of changes

----


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

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

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


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

asfgit
In reply to this post by asfgit
GitHub user clebertsuconic reopened a pull request:

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

    ARTEMIS-1495 No more Executor.flush!

   

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

    $ git pull https://github.com/clebertsuconic/activemq-artemis artemis-1495

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

    https://github.com/apache/activemq-artemis/pull/1650.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 #1650
   
----
commit 331c0b063d70eb0dbdb74e27a82d5e11a6ee364b
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-02T09:51:43Z

    ARTEMIS-1495 Test simulating a dead lock on queue auto create under stress

commit 613e6e460866b267839d660275ea848d3fde870a
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-07T19:52:19Z

    ARTEMIS-1495 Removing flushes from codebase
   
    Instead of flushing we just need to make sure there are no more calls into
    page executors as we stop the PageManager.
   
    This will avoid any possible starvations or deadlocks here.

commit d5504a3f1f7269c270a164c1c1a38616b421dd50
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-08T09:05:35Z

    ARTEMIS-1495 Sanity tests for the ProcessorBase::shutdownNow feature

commit 4ae6bf958c6186ee8d35f1a76bff1087e98c8d92
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-08T11:03:49Z

    ARTEMIS-1495 Lock-free ProcessorBase::shutdownNow

commit 67fa8570f524fc1f850c01ef73c004a803f0a3b5
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-08T14:16:59Z

    ARTEMIS-1495 Fixing In Handler executor and added benchmark to measure impact of changes

----


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    I am going to reopen this.. but will close it for now.


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

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

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


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    @franz1981 if you could take a look here please.. it's the last version I have...
   
    I will reopen this same PR once I have the testsuite done completely tomorrow.


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    @clebertsuconic I've just pushed a PR to your version with very few changes (but sadly seems more using diff on the code) mostly perf dependent and to make the code a lil more documented: I'm happy with your changes and the added test too, very well done!!!!! :+1:
   
    Let me know if the changes I've pushed seems good considering what are your expectations on the mechanics of the final version.
   
    Just a note (if possible): now we have some sanity tests on the OrderedExecutor hence I will be super happy to see the missing ones for the other critical methods, if any (eg: flush, shutdown)..if is not possible we'll add them later.


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

asfgit
In reply to this post by asfgit
GitHub user clebertsuconic reopened a pull request:

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

    ARTEMIS-1495 No more Executor.flush!

   

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

    $ git pull https://github.com/clebertsuconic/activemq-artemis artemis-1495

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

    https://github.com/apache/activemq-artemis/pull/1650.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 #1650
   
----
commit 331c0b063d70eb0dbdb74e27a82d5e11a6ee364b
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-02T09:51:43Z

    ARTEMIS-1495 Test simulating a dead lock on queue auto create under stress

commit 613e6e460866b267839d660275ea848d3fde870a
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-07T19:52:19Z

    ARTEMIS-1495 Removing flushes from codebase
   
    Instead of flushing we just need to make sure there are no more calls into
    page executors as we stop the PageManager.
   
    This will avoid any possible starvations or deadlocks here.

commit d5504a3f1f7269c270a164c1c1a38616b421dd50
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-08T09:05:35Z

    ARTEMIS-1495 Sanity tests for the ProcessorBase::shutdownNow feature

commit 4ae6bf958c6186ee8d35f1a76bff1087e98c8d92
Author: Francesco Nigro <[hidden email]>
Date:   2017-11-08T11:03:49Z

    ARTEMIS-1495 Lock-free ProcessorBase::shutdownNow

commit dc5a2d4a80f037cc139874d0a4684bf5f7fbf125
Author: Clebert Suconic <[hidden email]>
Date:   2017-11-08T14:16:59Z

    ARTEMIS-1495 Fixing In Handler executor and added benchmark to measure impact of changes

----


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

[GitHub] activemq-artemis pull request #1650: ARTEMIS-1495 No more Executor.flush!

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

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


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    I've merged this myself after extended review between me and @franz1981 , and extensive testings.
   
    We can send extra commits to master later if anything is still needed to be done.


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    @clebertsuconic well done!!!!!!


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

[GitHub] activemq-artemis issue #1650: ARTEMIS-1495 No more Executor.flush!

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

    https://github.com/apache/activemq-artemis/pull/1650
 
    @franz1981  you too :)


---