[
https://issues.apache.org/jira/browse/GROOVY-12398?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18114113#comment-18114113
]
ASF GitHub Bot commented on GROOVY-12398:
-----------------------------------------
paulk-asert commented on PR #2921:
URL: https://github.com/apache/groovy/pull/2921#issuecomment-5630235433
Initial AI read (my initial leaning is master only to start with and
potentially backport to 6 after some settling time):
> ## Assessment: PR 2921 (GROOVY-12398, lexer/parser performance)
>
> **Verdict: this is ~95% mechanically equivalent refactoring, but not 100%
— there are three genuine behavior deltas, two of which are bug fixes and one
of which is a deliberate parsing-strategy change. My recommendation: merge to
master, but don't pull it into the 6.0 RC line.**
>
> I read the full diff and hand-verified each change against the current
grammar. What's in it:
>
> **Verified mechanically equivalent (no behavior change):**
> - The number-literal fragment flattening (`Digits`, `HexDigits`,
`OctalDigits`, `BinaryDigits`, `ExponentPart`, `OctalEscape`, the `Dot`/`Zero`
fragment inlining) — I compared each old fragment chain against its new
character class and they match exactly. All references to the removed fragments
are replaced in the diff.
> - `isFollowedByWhiteSpaces` and `isFollowedByJavaLetterInGString`
regex-to-char-test rewrites — equivalent, including the subtle cases: Java's
ASCII-only `\s` (so `\u00A0` still counts as non-space), lone low surrogates,
and EOF (the old code reached `isJavaIdentifierPart(-1)` which happens to
return false; the new code guards it explicitly).
> - The ASCII identifier split (`[A-Z]` vs `[a-z$_]`) — the classes are
disjoint and their union equals the old ASCII `JavaLetter`, and the
BMP-non-ASCII alternatives reproduce the old predicate logic exactly.
>
> **Behavior deltas:**
>
> 1. **Supplementary-plane uppercase identifiers reclassify** from
`Identifier` to `CapitalizedIdentifier`. The old `CapitalizedIdentifier`
predicate tested `isUpperCase(_input.LA(-1))`, which for a surrogate pair sees
only the low surrogate — always false. The new rule tests the real code point.
So an identifier starting with, say, Deseret `𐐀` or mathematical `𝔄` (U+1D504)
now lexes as `CapitalizedIdentifier`. I checked the parser: the bare
`Identifier` token appears only inside the `identifier` rule (which accepts
both), so nothing that parses today breaks — but `className` accepts *only*
`CapitalizedIdentifier`, so something like `𝔄 x = 1` that today parses as a
command expression could now parse as a typed variable declaration.
Realistically this touches no real-world code, and the new behavior is arguably
the correct one, but it is technically a parse change, not pure performance.
>
> 2. **The `elementValues` predicate gate** (`@Foo(a = 1)` no longer
explored as an assignment expression). This one I checked hard because it's the
scariest on paper: the new `ELEMENT_VALUE_PAIR_NAME_TYPES` BitSet is *exactly*
the token set of `identifier ∪ keywords` (all 60 tokens including `MODULE`,
which is in `identifier` but not `keywords` — Daniel got that right). And since
the alternative order already made `elementValuePairs` win any ambiguity, and
any `name = expr` input viable as a single element value is also viable as a
pair (element values are a superset that includes expressions), the gate cannot
reject anything that parses today — worst case is a different error message on
already-invalid input. Safe.
>
> 3. **Invalid-octal error offset fix**: the old `invalidDigitCount` field
was never reset between tokens, so a second invalid octal in the same file got
a wrong caret position. The new `getText().length()` form is stateless and
correct. Error-reporting change only, and a fix.
>
> **Other observations:** CI is fully green across the entire matrix (all
LTS JDKs, three OSes, lint gate, CodeQL, SonarCloud). The PR adds decent
targeted tests for exactly the risky spots (identifier split incl. Unicode,
flattened literals incl. `07` invalid-octal boundary, annotation forms incl.
`@Foo(class = 1)` and `@Foo(a + 1)`). The ANTLR dependency is
`me.sunlan:antlr4` — Daniel's own fork — so prediction-time predicate semantics
for the new gate are under the author's control. The claimed 1.6× is
parse-only; end-to-end compile improvement will be a modest fraction of that,
though it does chip at the Groovy 6 vs 5.0.6 parse-phase gap we've catalogued.
>
> **Recommendation given RC phase:** By your own criterion this doesn't
qualify as "strictly performance" — items 1 and 3 change observable behavior in
edge cases, and grammar/ATN rewrites are inherently the highest-blast-radius
category of perf change (a lexer DFA subtlety that the test suite doesn't cover
would ship as a parse regression). None of the deltas endanger realistic code,
so I'd merge it to master for the next train without hesitation. For 6.0 in RC,
I'd defer: the win is parse-time only, and the cost of being wrong during RC
outweighs it. If you want it in 6.0 anyway, the minimum bar I'd suggest is one
more RC cycle after it lands plus a compile sweep of a large corpus (e.g. the
usual grails/spock reproducer set) comparing ASTs before/after.
> Improve lexer and parser performance
> ------------------------------------
>
> Key: GROOVY-12398
> URL: https://issues.apache.org/jira/browse/GROOVY-12398
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> The Groovy ANTLR4 grammar already uses left-recursive expressions, which is
> the main idea of the [optimized Java
> grammar|https://github.com/antlr/grammars-v4/tree/master/java/java]. Several
> remaining lexer and parser costs still match patterns that grammar was
> written to avoid.
> h3. Problem
> * Number literals are assembled from a chain of one-character fragments
> ({{Digit}}, {{Zero}}, {{NonZeroDigit}}, ...). The invalid-octal alternative
> also counts digits in a mid-rule action loop, which prevents DFA compilation
> of {{IntegerLiteral}}.
> * Every identifier, including ASCII names such as {{Foo}} and {{foo}},
> evaluates {{Character.isUpperCase}} to choose {{CapitalizedIdentifier}} vs
> {{Identifier}}.
> * GString and comment predicates ({{isFollowedByJavaLetterInGString}},
> {{isFollowedByWhiteSpaces}}) allocate a one-character {{String}} and run
> {{Pattern.matcher}} on each lookahead.
> * {{elementValues}} is ambiguous with assignment: {{@Foo(a = 1)}} can be
> named pairs or an assignment expression, so {{AdaptivePredict}} explores both
> (see [antlr/grammars-v4#215|https://github.com/antlr/grammars-v4/issues/215]).
> h3. Approach
> Keep token types and language semantics unchanged.
> * Flatten number and float fragments to character classes, as in the
> optimized Java lexer. Validate invalid octal once at the end of the
> alternative.
> * Split ASCII identifiers on a predicate-free DFA path: uppercase {{A-Z}}
> becomes {{CapitalizedIdentifier}}; {{a-z}}, dollar and underscore become
> {{Identifier}}. Unicode still uses predicates.
> * Replace the regex-based predicates with direct character tests.
> * Gate {{elementValue}} with {{!isIdentifierAssign(_input)}}, the same idea
> as Java's {{IsNotIdentifierAssign}}.
> h3. Result
> Local parse-only measurement (SLL then LL) of {{src/main/groovy}} +
> {{src/test-resources/core}} (245 files, approximately 570k characters):
> ||Run||Best||Mean||
> |before|0.231s|0.401s|
> |after|0.146s|0.169s|
> Best run is about 1.6x faster. Mean improves more because prediction is more
> stable after DFA warmup.
> Related: GROOVY-10454, GROOVY-9588
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
