[
https://issues.apache.org/jira/browse/QPID-3319?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13059237#comment-13059237
]
Robbie Gemmell commented on QPID-3319:
--------------------------------------
Hi Srinath, thanks for the patches. I have taken a look at them and it seems
fairly clear they should resolve the memory release issues with the current
implementation, however they do appear to introduce new issues and the general
approach appears to have scope for being significantly slower.
The new implementation utilizes an already thread-safe data structure in the
form of a LinkedConcurrentQueue but then applies synchronization to it,
seemingly in order to allow for cases such as checking the size in the face of
additions/removals or ensuring there is an elment present before requesting it
to avoid exceptions. In the former case tracking additions and removals like
the current implementation would seem to allow sizing without the
synchronization and would additionally avoid the introduced inefficiency of
traversing the entire structure in order to determine the size upon each
invocation of the size() method. In terms of checking for the non-empty case
before retrieving an element, there is an existing peek() method available on
the LinkedConcurrentQueue that provides the same functionality without the need
for synchronization to first check the size. The addition of synchronization in
general doesnt seem ideal for performance of the list, and I would suggest that
since use of the Iterator is not synchronized then its use for the other
methods has to be questioned. The weakly consistent nature of the Iterator also
looks to be the source of an issue whereby it can return elements that have
already been removed from the backing queue since the Iterator was created and
might not include new Subscriptions added since the Iterator was created, both
things the existing implementation makes effort to avoid.
The findNextNode(Subscription node) method seems like it should be a function
of the SubscriptionList itself, is there a reason it was added to the
SimpleAMQQueue? The requirement to search the list of subscriptions from the
beginning in order to find the next node is already less efficient than the
existing solution, but it appears that the method also always searches the
entire data set to the end rather than stopping as soon as it can identify the
next node. It isnt clear to me what special significance index == 2 provides
and along with it what exactly the 'alternativeNextNode' functionality is
doing, except possibly attempting to provide a fallback for when the provided
node isnt found? If so it doesnt appear this method will function in the
expected/existing manner (returning what would have been the next Subscription
to try immediate delivery to upon a new enqueue) in most situations that
occurs, and together with the changes in SimpleAMQQueue#enqueue could also
instead lead to specific subscriptions unfairly being used repeatedly instead
of cycling the subscriptions in the list.
The use of an existing generic data structure clearly leads to a vast
simplification of the SubscriptionList class itself, but I cant help but think
the above issues suggest retaining the existing approach whilst addressing its
memory issues might be the better option.
You mention having done intensive testing but I note there are no new tests
added by the patches. I would suggest a change of this type should really be
verfied through significant unit tests to help provide confidence in the
change. That the existing class has no unit tests is less than ideal, but
serves as all the more reason some should be added before/during large changes
to such a fairly mature and rarely changed piece of core code.
Finally, there are some stylistic issues in your patches such as use of tab
characters instead of spaces, and differences from our field naming convention.
The code style we use on the project is described at:
https://cwiki.apache.org/qpid/java-coding-standards.html
Regards,
Robbie
> org.apache.qpid.server.subscription.SubscriptionList leaks memory
> -----------------------------------------------------------------
>
> Key: QPID-3319
> URL: https://issues.apache.org/jira/browse/QPID-3319
> Project: Qpid
> Issue Type: Bug
> Reporter: Danushka Menikkumbura
> Assignee: Robbie Gemmell
> Priority: Critical
> Attachments: QPID-3319.2.patch, QPID-3319.3.patch, QPID-3319.patch
>
>
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
