[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
[
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17846767#comment-17846767
]
Paul Bjorkstrand commented on SLING-12279:
--
[~pakira], you are correct that this does not fix that particular problem. The
problem that this attempts to address, is when the cache fills with
[circularly-referenced cached
models|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector]
(most commonly when model marked with `cache=true`, and keeps a field with
`@Self`), this change should allow the GC to collect the cache when the
Resource Resolver or Request are closed/destroyed. That will eliminate a large
"sawtooth" GC graph.
Without this change, models that, directly or indirectly, reference their
adaptable will not be collected until there is memory pressure, and the GC
starts collecting SoftReference. I do believe with my proposed change, swapping
out the cache impl (currently just a `Map,
SoftReference>>` will be easier. A follow-on change could add in the
LRU portion of the cache impl in one place, rather than in the several it would
need today..
> Move Sling Model cache holder for Resource and ResourceResolver adaptables
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
> Attachments: cache-size.log, cachesize.log
>
>
> There have been several instances of issues with model caching over the
> years, the most recent being SLING-12259 and [this
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
> Jörg Hoh. These recent issues have been around cached items sticking
> around for too long. In that thread, it was discussed using
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
> whenever possible. This binds the cache to the lifecycle of the
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
> there is already a divergence that binds the cache for request-based models
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and
> use the resolver's property map to bind the cache for those adaptables to the
> lifecycle of the ResourceResolver. Resources are already (largely) bound to
> the lifecycle of the ResourceResolver that supplied them, and binding the
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the
> likelihood of the JVM's GC being put under pressure in cases when cached
> models [reference the original adaptable using
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
> As a bonus, if the cache object implements {{Closeable}}, the Sling
> Implementation will call {{close()}} on the cache when the resolver is
> closed,, giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
[
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17846765#comment-17846765
]
Sumanta Pakira commented on SLING-12279:
I think still the problem mentioned in SLING-12259 does not resolve. I tested
your code locally and attached is the cache entries which grows as the number
of request grows. The Softreference does not gets removed by the GC because
here the value of the cacheMap or the adapterCache simply grows over time.
[^cachesize.log]
Probably LRU cache could be useful.
> Move Sling Model cache holder for Resource and ResourceResolver adaptables
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
> Attachments: cache-size.log, cachesize.log
>
>
> There have been several instances of issues with model caching over the
> years, the most recent being SLING-12259 and [this
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
> Jörg Hoh. These recent issues have been around cached items sticking
> around for too long. In that thread, it was discussed using
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
> whenever possible. This binds the cache to the lifecycle of the
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
> there is already a divergence that binds the cache for request-based models
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and
> use the resolver's property map to bind the cache for those adaptables to the
> lifecycle of the ResourceResolver. Resources are already (largely) bound to
> the lifecycle of the ResourceResolver that supplied them, and binding the
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the
> likelihood of the JVM's GC being put under pressure in cases when cached
> models [reference the original adaptable using
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
> As a bonus, if the cache object implements {{Closeable}}, the Sling
> Implementation will call {{close()}} on the cache when the resolver is
> closed,, giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
[
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841341#comment-17841341
]
Paul Bjorkstrand commented on SLING-12279:
--
[~joerghoh], can you take a look at this and see if it solves your problem? I
believe it will, but I don't have a way to reliably test. I could try to
contrive some kind of unit test that does some verification, though it may
prove difficult to make. Since the GC is largely non-deterministic (from an
outsider's perspective), getting this into a field test would likely be better.
> Move Sling Model cache holder for Resource and ResourceResolver adaptables
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
>
> There have been several instances of issues with model caching over the
> years, the most recent being SLING-12259 and [this
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
> Jörg Hoh. These recent issues have been around cached items sticking
> around for too long. In that thread, it was discussed using
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
> whenever possible. This binds the cache to the lifecycle of the
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
> there is already a divergence that binds the cache for request-based models
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and
> use the resolver's property map to bind the cache for those adaptables to the
> lifecycle of the ResourceResolver. Resources are already (largely) bound to
> the lifecycle of the ResourceResolver that supplied them, and binding the
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the
> likelihood of the JVM's GC being put under pressure in cases when cached
> models [reference the original adaptable using
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
> As a bonus, if the cache object implements {{Closeable}}, the Sling
> Implementation will call {{close()}} on the cache when the resolver is
> closed,, giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
