qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robbie Gemmell (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (QPID-3319) org.apache.qpid.server.subscription.SubscriptionList leaks memory
Date Sun, 03 Jul 2011 17:15:21 GMT

    [ 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:dev-subscribe@qpid.apache.org


Mime
View raw message