[
https://issues.apache.org/jira/browse/SLING-10899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17770465#comment-17770465
]
Henry Kuijpers commented on SLING-10899:
----------------------------------------
[~joerghoh] [~cziegeler] There is no caching of the adaption, unfortunately,
and it's also not possible to do this outside of all the implementations:
Whenever ResourceResolver#getResource is called, a new Resource object is
returned. Never an existing one(? unless I'm missing something). It would be up
to ResourceResolverImpl to return the same one under certain conditions, if I
have that right. This would definitely be a nice upgrade (and I agree also a
complicated one), as right now, calling getResource() on the same instance of
ResourceResolver, will always return a new fresh Resource (which can be any
impl, of course).
In the case of a JCR Resource, a JcrNodeResource is returned. This
JcrNodeResource has 2 ways of obtaining a ValueMap:
* getValueMap
* adaptTo(ValueMap.class)
The getValueMap (coming from AbstractResource) calls adaptTo(ValueMap.class).
adaptTo(ValueMap.class) will *always* return a new instance of JcrValueMap, as
you can see here:
https://github.com/apache/sling-org-apache-sling-jcr-resource/blame/master/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResource.java#L136
It is not possible to influence this, as we have no influence over these
internal helper classes and/or any of the other impls.
So, this means that:
1. The ResourceResolverImpl is returning a fresh new JcrNodeResource object for
the same path *all the time*
2. The JcrNodeResource is returning a fresh new JcrValueMap for every call to
getValueMap/adaptTo(ValueMap.class) *all the time*
3. The cache that resides in JcrValueMap is of almost no use, as it will most
often have a cache miss.
4. It is not very easily possible to create a wrapper around such resources
(because, where would we create them)?
5. Creating a wrapping that could cache the valuemap is not possible, because
there are quite a few checks for "instanceof", which then isn't true anymore
A few very easy improvements could be:
1. When obtaining the ValueMap for a JcrNodeResource, create the map and save
it in an instance field, when calling it the next time, just return the
instance field, instead of creating a new JcrValueMap - This will of course all
go away when the Resource-object is garbage collected
2. Do not obtain the ValueMap in the adaptTo-method in the Resource impls, but
instead, use AdapterFactory for that, but, put some kind of caching mechanism
in there (basically a cache that could be similar to the Sling Models cache
that is in place)
3. Let the ValueMap-cache (and vielleicht other related stored data) live
together with the lifetime of the ResourceResolver, so as soon as it is closed,
also remove all the caches
In the improvements I don't see something so risky that it could blow the JVM
and/or cause too much memory to be claimed or similar. As long as it is
implemented well enough. :)
About doing the adaption repeatedly: We have no choice! If I have a Filter
implementation that needs to access the ValueMap of a Resource, I *need* to
call getValueMap and/or adaptTo(ValueMap.class). Both will have their own code
inside JcrNodeResource, that bypasses any AdapterFactory or anything else where
caching can take place. And *they do not cache anything*.
If the next Filter comes, and it has to access the ValueMap too, then the same
logic is kicked off and yet another new JcrValueMap (with empty cache) is
created, instead of reusing the previous one.
It is impossible to obtain a resource only once and reuse it throughout the
entire application and all the various phases.
Looking at every location independently is totally fine. I know most of the
locations that are interesting. But they always involve Sling/Adobe/... code,
mainly filters etc etc. And also our own code, but we have no way to pass those
Resource objects (and especially their JcrValueMap objects) around.
The problem is really inside JcrNodeResource and JcrValueMap. Please have
another look at it, based on this summary of the problem.
> Result of JcrNodeResource#adaptTo(ValueMap.class) should be cached
> ------------------------------------------------------------------
>
> Key: SLING-10899
> URL: https://issues.apache.org/jira/browse/SLING-10899
> Project: Sling
> Issue Type: Improvement
> Components: JCR
> Affects Versions: JCR Resource 3.0.22
> Reporter: Henry Kuijpers
> Priority: Major
>
> I was performance testing our application. There is a specific part of the
> code where we have a set of ~500 resources and we sort them based on a few
> properties of the resources. Multiple properties are used (which results in
> multiple calls to getValueMap). The sorting is done using a comparator, so
> the logic that determines the order is accessed numerous times.
> We've seen quite a decrease in performance when doing this with Resources
> that are of type JcrNodeResource. Turns out that the result of getValueMap
> (or adaptTo((Value)Map.class)) is not cached.
> As you can see in the adaptTo()-method implementation:
> https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/master/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResource.java#L136
> this call bypasses the AdapterFactory framework and also bypasses any
> caching that happens over there.
> What's even more unfortunate, is that JcrValueMap is implementing caching via
> https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/6e13f4d315ddf2804d2e16c55faee18e805b618e/src/main/java/org/apache/sling/jcr/resource/internal/JcrValueMap.java#L49
> . That caching is not leveraged properly, because every call to getValueMap
> returns a new JcrValueMap, instead of reusing a previously created one.
> One change that can be done is maybe converting the logic inside the
> adaptTo()-method to a dedicated AdapterFactory implementation, so that
> caching is done by default?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
