You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/12/24 17:54:28 UTC
[GitHub] activemq-artemis pull request #2480: ARTEMIS-2212 Avoid using CLQ on ServerC...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2480
ARTEMIS-2212 Avoid using CLQ on ServerConsumerImpl
It would deliver a better performance for the most
common operations eg offer, poll, iterations, size.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis array_q_vs_clq
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2480.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 #2480
----
commit b5cbf969225f8d72592d7da65e60b999e5ac6882
Author: Francesco Nigro <ni...@...>
Date: 2018-12-12T16:47:33Z
ARTEMIS-2212 Avoid using CLQ on ServerConsumerImpl
It would deliver a better performance for the most
common operations eg offer, poll, iterations, size.
----
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
If its not exposed and its used like doing lock on 'this". Then i can only assume it was the coders pattern at that time.
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
> Whats the concurrent performance on this. Do you have some stats?
Only allocations profiles and given that is in the hot path, we have a LOT of ConcurrentLinkedQueue::Node saved by this change.
The overall perf won't change by a bit (if not a tiny improvement), because it isn't a bottleneck AFAIK.
> Re sync lock blocks. Could be replaced with lock on this and or sync methods.
Agree at a first look, but I do not remember the reason why was used a separate lock TBH
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
@michaelandrepearce and @clebertsuconic in the new here please take a look at this one :+1:
I can change ArrayQueue with a LinkedList and I think that the benefits will be present due to avoiding a concurrent data structure, but we loose the added benefit of avoiding the garbage produced by the `Node` allocated.
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
@michaelandrepearce Not yet, need to chenge it and we just need the opinion from @clebertsuconic here, given that we I'm concerned about the max size that the array q can get and that will be retained for the whole life-cycle of the consumer :)
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
Whats the concurrent performance on this. Do you have some stats?
Re sync lock blocks. Could be replaced with lock on this and or sync methods.
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
@michaelandrepearce @clebertsuconic I have taken a better look to the code and seems that `deliveringRefs` is being limited (if set) by both `consumerWindowSize`and TCP-flow control hence I don't think is a big deal to use an `ArrayDeque` in place of a `LinkedList`, but I can use too a collection that get the bet of the both, a [ChunkedQueue](https://github.com/franz1981/activemq-artemis/blob/40fb24898b5a89445fd3e2743d0f369548a37a17/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/ChunkedQueue.java) that I have built some time ago. wdyt?
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
@franz1981, did you make the change to syncronized methods? As this PR is about performance im for getting the most we can in one ;)
---
[GitHub] activemq-artemis issue #2480: ARTEMIS-2212 Avoid using CLQ on ServerConsumer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2480
@michaelandrepearce I'm taking another look and I'm not 100% sure to replace the usage of the lock object with `this` or synchronized methods, because I see that the synchronization on `this` has a different semantic and uses, separated from `lock` eg
```java
public HandleStatus handle(final MessageReference ref) throws Exception {
// available credits can be set back to null with a flow control option.
AtomicInteger checkInteger = availableCredits;
if (callback != null && !callback.hasCredits(this) || checkInteger != null && checkInteger.get() <= 0) {
if (logger.isDebugEnabled()) {
logger.debug(this + " is busy for the lack of credits. Current credits = " +
availableCredits +
" Can't receive reference " +
ref);
}
return HandleStatus.BUSY;
}
synchronized (lock) {
```
`handle` is not synchronized, while other methods are, but a part of it `synchronized (lock)`, making the behaviour different if I replace `synchronized (lock)` with a `synchronized (this)` ie the risk is to be blocked by other method calls that currently are not blocking it
---