[jira] [Commented] (QPID-3319) org.apache.qpid.server.subscription.SubscriptionList leaks memory

["Robbie Gemmell (JIRA)" <ji...@apache.org>]

    [ 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