[
https://issues.apache.org/jira/browse/GROOVY-12316?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109102#comment-18109102
]
ASF GitHub Bot commented on GROOVY-12316:
-----------------------------------------
paulk-asert opened a new pull request, #2840:
URL: https://github.com/apache/groovy/pull/2840
AtnManager has two mechanisms for dropping the shared parser DFA cache, but
they were mutually exclusive. isSmartCleanupEnabled() is defined as
DFA_CACHE_THRESHOLD == 0, and the canary clear was guarded by
shouldClearDfaCache() && isSmartCleanupEnabled(), so setting any positive
groovy.antlr4.cache.threshold switched the GC canary off.
Since the threshold is the documented knob for bounding DFA cache growth,
reaching for it to reduce memory use silently removed the only mechanism that
responds to actual memory pressure. This was hit in practice: a user chasing an
OOM on 5.1.1 under Maven at -Xmx128m set threshold=200 as a mitigation, which
made 5.1.0 start OOMing too because it disabled the valve that had been keeping
it alive.
Derive two independent switches from the raw property instead of overloading
one: GC_CANARY_ENABLED (t >= 0) and DFA_CACHE_THRESHOLD (max(t, 0)). Behaviour
changes only for a positive threshold, which now keeps the canary as well as
the counter. Zero (the default) is unchanged, so there is no throughput cost
for users who do not set the property, and a negative value remains the
explicit "never clear" escape hatch — it now switches off both mechanisms,
since the canary is no longer implied by a zero threshold.
The forked-JVM test fails without the fix: with threshold=25 and only 12
parses the counter cannot fire, and after the JVM clears soft references the
parser DFA state count goes 478 -> 486 instead of dropping.
This does not on its own close the 5.1.1 memory regression. At the default
the canary is still only observed on the parse path, so under a tight heap the
cache can go unreclaimed until an OOM that arrives mid-parse.
> groovy.antlr4.cache.threshold silently disables pressure-based DFA clearing
> ---------------------------------------------------------------------------
>
> Key: GROOVY-12316
> URL: https://issues.apache.org/jira/browse/GROOVY-12316
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Problem
> {{AtnManager}} has two mechanisms for dropping the shared parser DFA cache,
> but they are mutually exclusive rather than composable:
> * a *GC canary* — the softly referenced {{AtnWrapper}}. Its collection
> signals memory pressure, so the cache is dropped when the next parse observes
> the cleared reference.
> * a *parse counter* — clears every {{groovy.antlr4.cache.threshold}} parses.
> {{isSmartCleanupEnabled()}} is defined as {{DFA_CACHE_THRESHOLD == 0}}, and
> the canary clear is guarded by {{shouldClearDfaCache() &&
> isSmartCleanupEnabled()}}. Setting *any* positive threshold therefore
> switches the canary off.
> Since the threshold is the documented knob for bounding DFA cache growth, a
> user reaching for it to reduce memory use silently removes the only mechanism
> that responds to actual memory pressure — and can end up worse off than
> before.
> This was hit in practice: a user chasing an OOM on 5.1.1 under Maven at
> {{-Xmx128m}} set {{-Dgroovy.antlr4.cache.threshold=200}} as a mitigation,
> which made *5.1.0* start OOMing too, because it disabled the valve that had
> been keeping it alive.
> h2. Reproduction
> In a forked JVM with {{-Dgroovy.antlr4.cache.threshold=25}}, parse 12 sources
> (fewer than the threshold, so the counter cannot fire), force the JVM to
> clear soft references, then parse once more and count live states in
> {{GroovyParser._ATN.decisionToDFA}}:
> || || DFA states populated || after soft refs cleared ||
> | current | 478 | 486 — not reclaimed |
> | composed | 478 | drops — reclaimed by the canary |
> h2. Fix
> Derive two independent switches from the raw property instead of overloading
> one:
> {code:java}
> GC_CANARY_ENABLED = gcCanaryEnabled(t); // t >= 0
> DFA_CACHE_THRESHOLD = counterThreshold(t); // max(t, 0)
> {code}
> The canary clear is then guarded by {{GC_CANARY_ENABLED}} and the parse
> counter by {{isThresholdCleanupEnabled()}}, so both can be active at once.
> || threshold || before || after ||
> | {{0}} (default) | canary on, counter off | unchanged |
> | {{> 0}} | canary *off*, counter on | canary *on*, counter on |
> | {{< 0}} | never clear | unchanged (never clear) |
> Behaviour changes only in the {{> 0}} case. The negative value remains the
> explicit "never clear" escape hatch — it now switches off both mechanisms,
> since "canary on" is no longer implied by a zero threshold.
> h2. Notes
> * The default configuration is unchanged, so this carries no throughput cost
> for users who do not set the property.
> * This makes the knob usable; it does not on its own close the 5.1.1 memory
> regression. At the default the canary is still only observed on the parse
> path, so under a tight heap the cache can go unreclaimed until an OOM that
> arrives mid-parse. Closing that needs either a non-zero default threshold or
> holding the DFA cache softly in the ANTLR runtime.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
