You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "Martijn Hendriks (JIRA)" <ji...@apache.org> on 2007/02/02 15:42:05 UTC
[jira] Created: (JCR-731) Can the caching mechanism be improved?
Can the caching mechanism be improved?
--------------------------------------
Key: JCR-731
URL: https://issues.apache.org/jira/browse/JCR-731
Project: Jackrabbit
Issue Type: Improvement
Reporter: Martijn Hendriks
Hi all,
We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
Regards,
Martijn Hendriks
<GX> creative online development B.V.
t: 024 - 3888 261
f: 024 - 3888 621
e: martijnh@gx.nl
Wijchenseweg 111
6538 SW Nijmegen
http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-731) Can the caching mechanism be improved?
Posted by "Martijn Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martijn Hendriks updated JCR-731:
---------------------------------
Attachment: JCR-731.diff
Hi Stefan,
Here is the diff w.r.t JackRabbit 1.2.1. Please note that we also applied Jaka Jaksic's patch for JCR-725 to the CacheManager.
Martijn
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, JCR-731.diff, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-731) Can the caching mechanism be improved?
Posted by "Jukka Zitting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jukka Zitting updated JCR-731:
------------------------------
Status: Patch Available (was: Open)
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit Content Repository
> Issue Type: Improvement
> Components: jackrabbit-core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, CachingTest.java, JCR-731.diff, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-731) Can the caching mechanism be improved?
Posted by "Martijn Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martijn Hendriks updated JCR-731:
---------------------------------
Component/s: core
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-731) Can the caching mechanism be improved?
Posted by "Martijn Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martijn Hendriks updated JCR-731:
---------------------------------
Attachment: CachingTest.java
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, CachingTest.java, JCR-731.diff, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-731) Can the caching mechanism be improved?
Posted by "Stefan Guggisberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12469776 ]
Stefan Guggisberg commented on JCR-731:
---------------------------------------
martijn, please provide a diff rather than the full source. that way it would be easier to review.
thanks
stefan
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-731) Can the caching mechanism be improved?
Posted by "Martijn Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Martijn Hendriks updated JCR-731:
---------------------------------
Attachment: MLRUItemStateCache.java
CacheManager.java
SharedItemStateManager.java
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-731) Can the caching mechanism be improved?
Posted by "Stefan Guggisberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12470254 ]
Stefan Guggisberg commented on JCR-731:
---------------------------------------
first of all, the patch looks good in general, great job martijn!
however, a few comments:
SharedItemStateManager.hasNonVirtualItemState():
the hasItemState() method was introduced as a performance improvement in order to avoid having to catch
NoSuchItemStateException when checking existence using getItemState() method. your suggestion
re-introduces catching exceptions...
general:
the patch is hardly trivial and affects crucial core components. especially MLRUItemStateCache.shrinkIfRequired()
and CacheManager.resizeAll() are significantly more complex and bear the risk of introducing new issues/bottlenecks.
code style:
there are some (minor) isssues related to code style. the patch uses tab characters rather than spaces
for indenting. some changes were pure reformatting (replacing spaces by tabs) which makes it unnecessary
hard to read the patch. another issue is operator padding: we use spaces around operators, e.g.
"if (x >= 0) " rather than "if (x>=0)".
before applying the suggested changes i'd like to see some simple benchmark results/simple test cases that show
the overall performance imrovement.
again thanks for your contribution!
cheers
stefan
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, JCR-731.diff, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-731) Can the caching mechanism be improved?
Posted by "Martijn Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-731?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472398 ]
Martijn Hendriks commented on JCR-731:
--------------------------------------
Stefan, thanks for your feedback!
I agree that using the exception mechanism for checking the existence of item states should be avoided. I noticed, however, that the commen code pattern "if (node.hasNode("a")) Node aNode = node.getNode("a"); " uses 4 calls to the database if the node "a" exists, but is not yet in the cache. The pre-loading in hasNonVirtualItemState reduces this to only 1 call. The usage of an exception can quite easily be avoided, I think, by letting the persistence manager not throw an "NoSuchItemStateException" if a state doesn't exist, but instead just return null.
With pre-loading there is of course the drawback that a huge state is loaded, but not requested by the repository user. I have the feeling that this is uncommon.
The patch indeed is non-trivial and needs carefull reviewing. I unfortunately did not take the Jackrabbit code style into account... I'm happy to provide a fixed patch that solves this if needed.
As for the benchmark you mention, I will attach a test class that spawns a number of threads that randomly read from a repository. Especially the impact of the pre-loading is significant: the test runs approximately 1.7 times as fast. Additional use of the MLRU and CacheManager patch improves this a bit to 1.8.
Best wishes,
Martijn
> Can the caching mechanism be improved?
> --------------------------------------
>
> Key: JCR-731
> URL: https://issues.apache.org/jira/browse/JCR-731
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Martijn Hendriks
> Attachments: CacheManager.java, JCR-731.diff, MLRUItemStateCache.java, SharedItemStateManager.java
>
>
> Hi all,
> We've identified the method "getNonVirtualItemState" in the SharedItemStateManager as a hot spot in our application. To avoid the contention in "getNonVirtualItemState", we have pulled the isCached call out of the synchronized block and re-implemented the MLRUItemStateCache. It uses a HashMap that contains the ItemId-ItemState mapping and a ReadWriteLock to replace all synchronized blocks in the code. The implementation of "shrinkIfRequired" unfortunatly got much more expensive as the entryset of the HashMap must be sorted by accesscount. This method then clearly is a bottleneck. We solved this by changing the CacheManager a bit: the "resizeAll" method avoids the eviction of items out of caches as long as possible.
> These changes work out really well for our application. I have attached the changed files; comments/feedback are very welcome!
> Regards,
> Martijn Hendriks
> <GX> creative online development B.V.
>
> t: 024 - 3888 261
> f: 024 - 3888 621
> e: martijnh@gx.nl
>
> Wijchenseweg 111
> 6538 SW Nijmegen
> http://www.gx.nl/
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.