[
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: [EMAIL PROTECTED]
>
> 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.
