[
https://issues.apache.org/jira/browse/GROOVY-12344?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18111372#comment-18111372
]
ASF GitHub Bot commented on GROOVY-12344:
-----------------------------------------
paulk-asert merged PR #2868:
URL: https://github.com/apache/groovy/pull/2868
> Replace TemplateServlet backend cache class
> -------------------------------------------
>
> Key: GROOVY-12344
> URL: https://issues.apache.org/jira/browse/GROOVY-12344
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> {{TemplateServlet}} caches compiled templates in a {{WeakHashMap}} that is
> read and written from {{service()}} with no synchronization:
> {code:java}
> private final Map<String, TemplateCacheEntry> cache;
> ...
> this.cache = new WeakHashMap<String, TemplateCacheEntry>();
> {code}
> There are two independent defects here. The caching one is the more
> consequential.
> h3. 1. The cache almost never hits
> The key is {{file.getAbsolutePath()}} (or {{url.toString()}}), a {{String}}
> local to {{getTemplate}}. {{WeakHashMap}} holds its _keys_ weakly, so an
> entry survives only while something outside the map strongly references that
> key. Nothing does: {{TemplateCacheEntry}} stores only {{lastModified}},
> {{length}}, a {{Date}} and the {{Template}} — deliberately not the {{File}} —
> so the key becomes unreachable the moment the request returns.
> Every entry is therefore collectible as soon as it is created, and the cache
> empties at the next GC regardless of how hot a template is:
> {noformat}
> key identity reused across requests? false (equal: true)
> cache size right after request: 1
> cache size after GC: 0
> next request hits cache? false
> {noformat}
> Note when reproducing this: build the path at runtime. Deriving it from a
> source literal makes the entry appear to survive, because the literal is
> interned and permanently reachable from the constant pool. A servlet's path
> always comes from the request URI.
> h3. 2. Concurrent requests can corrupt the map
> {{HashMap}} received the JDK 8 rewrite that ended the well-known resize
> infinite-loop. {{WeakHashMap}} did not — as of JDK 17 its {{transfer()}}
> still head-inserts:
> {code:java}
> int i = indexFor(e.hash, dest.length);
> e.next = dest[i];
> dest[i] = e;
> {code}
> That is the list reversal which lets two concurrent resizers build a cycle,
> after which {{get()}} spins forever. The mechanism is present in the shipped
> JDK; a spin has not been forced under test, as it is timing dependent.
> {{TemplateCacheEntry.hit}} is likewise a non-atomic {{long}} incremented from
> concurrent requests, so hit counts in verbose mode can be wrong.
> h3. Proposed fix
> Use {{ConcurrentHashMap}}. Keep the plain {{get}}/{{put}} pair rather than
> {{computeIfAbsent}}: template compilation is slow and would hold a bin lock
> for its duration, and two threads racing to compile the same template is
> harmless and rare.
> Strong references mean templates are now retained for the servlet's lifetime,
> which is the point — the key space is bounded by templates that actually
> exist, since {{service()}} sends a 404 before {{getTemplate}} is reached in
> both the {{File}} branch ({{exists()}}/{{canRead()}}) and the {{URL}} branch
> ({{getResource(name) == null}}). {{getScriptUri}} is overridable, but an
> override still has to name a resolvable resource, so a subclass cannot make
> the key space unbounded.
> Staleness handling is unaffected: {{TemplateCacheEntry.validate()}} continues
> to check {{lastModified}} and {{length}}, so an edited template is still
> picked up.
> h3. Throughput
> Read throughput, 40 cached templates, JDK 17:
> ||Map||1 thread||4 threads||8 threads||
> |{{WeakHashMap}} (as shipped)|53.7 M gets/s|corrupts|corrupts|
> |{{synchronizedMap(WeakHashMap)}}|55.0 M gets/s|16.4 M gets/s|18.2 M gets/s|
> |{{ConcurrentHashMap}}|70.9 M gets/s|284.0 M gets/s|584.8 M gets/s|
> {{ConcurrentHashMap}} is ahead even single-threaded, because
> {{WeakHashMap.get}} polls a {{ReferenceQueue}} on every read and dereferences
> a {{WeakReference}} per probe. Wrapping the existing map in
> {{synchronizedMap}} would fix the corruption but leaves a cache that does not
> cache, and goes backwards under contention.
> h3. Compatibility
> No API change. {{cache}} is a {{private final}} field whose declared type
> stays {{Map}}, {{TemplateCacheEntry}} is a {{private static}} class, and the
> two helpers are {{private}} — only the constructor body changes, so the
> change is source and binary compatible and existing compiled subclasses link
> unchanged. The only operations on the map are {{get}} and {{put}}, so no
> iteration or view semantics are observable.
> {{ConcurrentHashMap}} rejects null keys and values, which cannot arise here:
> {{service()}} validates the file and the URL before {{getTemplate}} is
> called, and {{TemplateCacheEntry}}'s constructor throws on a null template.
> One residual difference: {{HttpServlet}} is {{Serializable}} and {{cache}} is
> not transient, while {{TemplateCacheEntry}} and {{Template}} are not
> serializable. Today the cache is usually empty, so serializing a
> {{TemplateServlet}} can succeed by chance; with a populated cache it would
> consistently throw {{NotSerializableException}}. Containers passivate
> sessions rather than servlet instances, so this is theoretical, and
> {{transient}} is not a free fix given the field is {{final}} and
> deserialization skips the constructor.
> The cache was introduced as a {{WeakHashMap}} in 2005 (GROOVY-814), in a
> revision still carrying {{// Java5}} comments beside the raw-typed field. No
> rationale was recorded.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
