[
https://issues.apache.org/jira/browse/GROOVY-12344?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110923#comment-18110923
]
ASF GitHub Bot commented on GROOVY-12344:
-----------------------------------------
paulk-asert opened a new pull request, #2868:
URL: https://github.com/apache/groovy/pull/2868
…tHashMap
The cache was a WeakHashMap keyed on file.getAbsolutePath() or
url.toString(). Both are freshly built strings local to getTemplate, and
TemplateCacheEntry retains only lastModified, length, a Date and the Template,
never the File, so nothing outside the map referred to a key once its request
returned. Weakly held keys made every entry collectible as soon as it was
stored, and the cache emptied at each collection however hot a template was.
The map was also read and written from service() without synchronization.
WeakHashMap did not receive the JDK 8 rewrite that ended the HashMap resize
infinite-loop, so its transfer() still head-inserts and concurrent resizes can
build a cycle that leaves get() spinning.
ConcurrentHashMap addresses both, and is faster besides: WeakHashMap.get
polls a ReferenceQueue on every read and dereferences a WeakReference per
probe. Templates are now retained for the servlet's lifetime, which is bounded
by the templates that exist, since service() answers 404 before getTemplate is
reached for a missing file or unresolvable resource. Staleness handling is
unchanged, as validate() still compares lastModified and length.
The cache field is private and of declared type Map, TemplateCacheEntry is
private, and only get and put are used, so the change is source and binary
compatible.
> 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)
