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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
