You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "Alex Parvulescu (Created) (JIRA)" <ji...@apache.org> on 2011/11/22 15:00:41 UTC
[jira] [Created] (JCR-3151) SharedFieldCache can cause a memory
leak
SharedFieldCache can cause a memory leak
----------------------------------------
Key: JCR-3151
URL: https://issues.apache.org/jira/browse/JCR-3151
Project: Jackrabbit Content Repository
Issue Type: Bug
Components: jackrabbit-core
Reporter: Alex Parvulescu
The SharedFieldCache has some problems with the way it builds the cache:
- as key is has the IndexReader
- as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
This 'Key' holds a reference to the comparator used for in the queries ran.
Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
So the circle is complete and the SharedFieldCache entries never get GC'ed.
One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Alex Parvulescu (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155528#comment-13155528 ]
Alex Parvulescu commented on JCR-3151:
--------------------------------------
> The patch might break things in the situation where different ValueIndex instances should go into the cache for the same field and same prefix but a for a different FieldComparator implementation: despite different, both ValueIndex instances end up at the same cache slot.
Agreed, what I cannot understand is the relation between the ValueIndex and the provided field comparator implementation. To me they are 2 different things. Just look at how the ValueIndex cache is being build.
So I'm not sure if them "ending up at the same cache slot" is actually a bad thing :)
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Alex Parvulescu (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alex Parvulescu resolved JCR-3151.
----------------------------------
Resolution: Fixed
Fix Version/s: 2.3.4
Assignee: Alex Parvulescu
Fixed in revision 1205361.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Assignee: Alex Parvulescu
> Fix For: 2.3.4
>
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Marcel Reutegger (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155797#comment-13155797 ]
Marcel Reutegger commented on JCR-3151:
---------------------------------------
Hmm, I can't remember why the comparator source is part of the Key. I agree with both of you that it isn't necessary because it has no influence on the ValueIndex that is created.
+1 for the patch.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Michael Dürig (Commented JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155350#comment-13155350 ]
Michael Dürig commented on JCR-3151:
------------------------------------
Testing indicates that the patch indeed works: Without the patch applied, executing a query with an order by clause in a loop will eat up memory very quickly and eventually result in an OOME. With the patch applied, I can see memory decreasing up to a certain limit where GC kicks in an the lower bound on free memory keeps stable. Also the jackrabbit-core test cases run. So I'm all fine with the patch.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Michael Dürig (Commented JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155324#comment-13155324 ]
Michael Dürig commented on JCR-3151:
------------------------------------
After having looked at the code and the patch this looks good to me so far. I think the conclusion regarding granularity of equals of the Key class holds. That is, there is no harm in removing the comperator field from the Key class.
The comperator field has been part of the Key class since beginning of times (since JCR-106 that is). Maybe Marcel still remembers the rational for this.
I will still do some testing of the patch to see whether it actually fixes the memory leak and document my findings here later.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Michael Dürig (Commented JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155538#comment-13155538 ]
Michael Dürig commented on JCR-3151:
------------------------------------
> So I'm not sure if them "ending up at the same cache slot" is actually a bad thing :)
Yes same here. That's way I asked Marcel to comment on this. AFAIK the initial code is from him.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Michael Dürig (Commented JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155493#comment-13155493 ]
Michael Dürig commented on JCR-3151:
------------------------------------
On a second though removing the comperator field from the Key class might have some subtle side effects. The patch might break things in the situation where different ValueIndex instances should go into the cache for the same field and same prefix but a for a different FieldComparator implementation: despite different, both ValueIndex instances end up at the same cache slot.
To be on the safe side I think the comperator should thus 'somehow' stay in the Key class. One solution might be to use a WeakReference instead the comperator instance itself. Another (a bit hacky) solution might be to use the comepator's hashCode instead of the comperator instance itself.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Michael Dürig (Commented JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155547#comment-13155547 ]
Michael Dürig commented on JCR-3151:
------------------------------------
I tested both my suggestions from above. Both works so far. With the WeakReference based approach one has to take care to use a super class which overrides equals and hashCode delegating to the referent. Also this approach seems heavier on GC: I see more GC cycles than with the hash code based approach.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Alex Parvulescu (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alex Parvulescu updated JCR-3151:
---------------------------------
Attachment: JCR-3151.patch
attached patch.
fair warning;, it also contains some cosmetics involving Comparable (my eyes hurt from all those warnings in eclipse :)
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Alex Parvulescu (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155799#comment-13155799 ]
Alex Parvulescu commented on JCR-3151:
--------------------------------------
thanks for the feedback guys!
I'll apply the patch shortly.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (JCR-3151) SharedFieldCache can cause a memory
leak
Posted by "Alex Parvulescu (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-3151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13155798#comment-13155798 ]
Alex Parvulescu commented on JCR-3151:
--------------------------------------
If we decide to keep any kind of reference to the comparator instance, I'd go with the hash code approach.
It seems really lightweight and it provides enough uniqueness for the cache key to avoid potential collision.
> SharedFieldCache can cause a memory leak
> ----------------------------------------
>
> Key: JCR-3151
> URL: https://issues.apache.org/jira/browse/JCR-3151
> Project: Jackrabbit Content Repository
> Issue Type: Bug
> Components: jackrabbit-core
> Reporter: Alex Parvulescu
> Attachments: JCR-3151.patch
>
>
> The SharedFieldCache has some problems with the way it builds the cache:
> - as key is has the IndexReader
> - as value it has a inner cache (another map) that has as a key a static inner class called 'Key'.
> This 'Key' holds a reference to the comparator used for in the queries ran.
> Assuming this comparator is of any type that extends from AbstractFieldComparator (I think all of the custom JR comparators), then it keeps a reference to all the InderReader instances in order to be able to load the values as Comparable(s).
> So the circle is complete and the SharedFieldCache entries never get GC'ed.
> One option would have been to implement a 'purge' method on the cache, similar to the lucene mechanism, and when an InderReader gets closed is could call 'purge'. But that is both ugly AND is doesn't seem to work that well :)
> A more radical option is to remove the cache completely. Each instance of SimpleFieldComparator (the only client of this cache) already builds an array of the available values, so the cache would only help other instances of the same type. We'll not analyze this further.
> The proposed solution (patch will follow shortly) is to remove the Comparator reference from the Key class.
> It looks like it has no real purpose there, just to impact the 'equals' of the key, which makes no sense in the first place as the lucene query does not use the Comparator info at all.
> If anything, using the same field and 2 different Comparators we'll get 2 different cache entries based on the same values from the lucene index.
> Feedback is appreciated!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira