Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-2212 Avoid using CLQ on ServerConsumerImpl #2480

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@franz1981
Copy link
Contributor

franz1981 commented Dec 24, 2018

It would deliver a better performance for the most
common operations eg offer, poll, iterations, size.

ARTEMIS-2212 Avoid using CLQ on ServerConsumerImpl
It would deliver a better performance for the most
common operations eg offer, poll, iterations, size.
@franz1981

This comment has been minimized.

Copy link
Contributor Author

franz1981 commented Dec 24, 2018

@michaelandrepearce and @clebertsuconic in the new year please take a look at this one 👍
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.

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

michaelandrepearce commented Dec 24, 2018

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.

@franz1981

This comment has been minimized.

Copy link
Contributor Author

franz1981 commented Dec 24, 2018

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

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

michaelandrepearce commented Dec 26, 2018

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.

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

michaelandrepearce commented Jan 3, 2019

@franz1981, did you make the change to syncronized methods? As this PR is about performance im for getting the most we can in one ;)

@franz1981

This comment has been minimized.

Copy link
Contributor Author

franz1981 commented Jan 3, 2019

@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 :)

@franz1981

This comment has been minimized.

Copy link
Contributor Author

franz1981 commented Jan 5, 2019

@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

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 has a 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

@franz1981

This comment has been minimized.

Copy link
Contributor Author

franz1981 commented Jan 9, 2019

@michaelandrepearce @clebertsuconic I have taken a better look to the code and seems that deliveringRefs is being limited (if set) by both consumerWindowSizeand 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 best of both, a ChunkedQueue that I have built some time ago. wdyt?

while (iter.hasNext()) {
MessageReference ref = iter.next();

refs.forEach(ref -> {

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Jan 17, 2019

Contributor

is this atomic?

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Jan 17, 2019

Contributor

Ohhh... ok.. it's on the list from cancelRefs.. I confused as being the main list.

@asfgit asfgit closed this in 4179c44 Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.