[
https://issues.apache.org/jira/browse/SLING-10899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17437160#comment-17437160
]
Carsten Ziegeler commented on SLING-10899:
------------------------------------------
I think we had a lot of discussions around this in the past - if my memory
serves me well, we lazily agreed that an adaption of an object to another
object should return the same instance - if possible. That's why we added the
caching in SlingAdaptable. Of course there are edge cases like adapting to an
InputStream where caching doesn't make sense.
So looking at it from this POV, the current behaviour (returning a new
ValueMap) seems to be a bug - and returning the same ValueMap is more
consistent. For read-only cases this should not matter anyway but improves
performance.
For write cases, it gets more tricky - I'm not 100% sure there - although
having a consistent view of the changed content looks like the way to go, so
again returning the same object seems to be the right thing.
But there is a higher level problem which will not be solved by this - and in
the past we lazily agreed in the past to not solve it: two Resource objects
representing the same resource will result different ValueMap objects - and
therefore (in the write case) potentially different state, like:
{noformat}
Resource a = resolver.getResource("/foo");
ValueMap va = a.adaptTo(ValueMap.class);
Resource b = resolver.getResource("/foo");
ValueMap vb = b.adaptTo(ValueMap.class);
{noformat}
in that case va != vb (same for the modifiable value map)
> 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
> Assignee: Carsten Ziegeler
> Priority: Major
> Fix For: JCR Resource 3.0.24
>
>
> 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.3.4#803005)
