release 0.2.2: perf rework (4.23× speedup)#13
Merged
Merged
Conversation
📝 WalkthroughWalkthroughIntroduced a major performance rework framework for the PEG CST parser generator, featuring generator-time Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR spans a substantial and dense refactor of Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
src/test/resources/perf-corpus/flow-format-examples/BlankLineRules.java (2)
26-26: Minor formatting inconsistency in record declaration.The record declaration is missing a space before the opening brace. Standard Java formatting conventions typically include a space.
✨ Suggested formatting improvement
- record Inner(String value){} + record Inner(String value) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/resources/perf-corpus/flow-format-examples/BlankLineRules.java` at line 26, The record declaration for Inner lacks a space before the opening brace; update the declaration of record Inner(String value) {} (symbol: Inner) to include a space before the '{' so it follows standard Java formatting conventions (i.e., change "record Inner(String value){}" to "record Inner(String value) {}").
6-6: Note: Unused fieldactivein test fixture.The
activefield is initialized in the constructor but never exposed via a getter or used elsewhere in the class. This is acceptable for test fixture data, but if this field is intended to be accessed, consider adding a getter method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/resources/perf-corpus/flow-format-examples/BlankLineRules.java` at line 6, The private field `active` in class `BlankLineRules` is never used outside the constructor; either remove the unused field or add a public accessor if it should be exposed—locate the `private final boolean active;` declaration and the constructor that initializes it, then either delete the field and related constructor parameter or implement a getter method (e.g., `isActive()` or `getActive()`) so callers can read the value.src/test/java/org/pragmatica/peg/perf/CstReconstruct.java (1)
85-93:setAccessible(true)is unnecessary for public record accessors.The CST node types are records with public accessor methods.
setAccessible(true)is only needed for non-public members and adds slight overhead. Since this is test utility code, it's not critical, but it could be removed.♻️ Optional simplification
private static Method findMethod(Class<?> cls, String name) { try { var m = cls.getMethod(name); - m.setAccessible(true); return m; } catch (NoSuchMethodException e) { throw new RuntimeException("No method " + name + "() on " + cls, e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/pragmatica/peg/perf/CstReconstruct.java` around lines 85 - 93, The helper findMethod(Class<?> cls, String name) calls m.setAccessible(true) unnecessarily for public record accessor methods; remove the m.setAccessible(true) call and simply return the Method obtained from cls.getMethod(name), keeping the existing try/catch and RuntimeException behavior for NoSuchMethodException so tests still fail cleanly when the accessor is missing.docs/bench-results/java25-parse.json (1)
8-13: Normalize host-specific JVM path in committed benchmark snapshots.Committing absolute local paths makes this artifact noisier and less portable; prefer a sanitized value (e.g., just JDK/VM version fields).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/bench-results/java25-parse.json` around lines 8 - 13, The benchmark snapshot currently commits a host-specific absolute JVM path under the "jvm" key which makes the artifact non-portable; change the snapshot generation (or post-processing) so that the "jvm" field is normalized to a generic value (e.g., null, "", or a placeholder like "JVM_PATH_OMITTED") or omitted entirely while retaining the actual version details in "jdkVersion", "vmName", and "vmVersion"; update the code that writes the JSON (the logic producing the "jvm" key) to substitute the sanitized value instead of the absolute path and ensure tests or consumers expect the normalized form.docs/bench-results/packrat-stats.txt (1)
1-2: Drop machine-specific warning preamble from committed benchmark output.These lines are environment noise and reduce artifact stability across contributors/CI runs.
Proposed cleanup
-Note: /var/folders/m7/lwdzwk711cl83l1tf25ssnkm0000gn/T/peglib-bench-generated11399443917318641120/generated/bench/probe/Java25ProbeParser.java uses or overrides a deprecated API. -Note: Recompile with -Xlint:deprecation for details. === PackratStatsProbe ===🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/bench-results/packrat-stats.txt` around lines 1 - 2, The committed benchmark output contains machine-specific compiler warnings at the top that add noise; remove the two preamble lines ("uses or overrides a deprecated API" and "Recompile with -Xlint:deprecation for details") from docs/bench-results/packrat-stats.txt, or update the benchmark generation workflow to strip such stderr/stdout warnings before committing (i.e., filter these exact warning patterns from generated bench output so the artifact is stable across machines).src/test/java/org/pragmatica/peg/perf/Phase2MarkResetChildrenParityTest.java (1)
52-80: Consider extracting shared test infrastructure.The
corpusFiles()method and the baseline-loading/comparison logic are duplicated across all 9 parity test classes. Extracting this to a shared base class or utility would reduce maintenance burden and ensure consistent behavior.Example extraction:
// In a new ParityTestSupport.java public final class ParityTestSupport { public static final Path CORPUS_ROOT = Path.of("src/test/resources/perf-corpus"); public static final Path BASELINE_ROOT = Path.of("src/test/resources/perf-corpus-baseline"); public static Stream<Path> corpusFiles() throws IOException { ... } public static String loadBaseline(Path corpusFile) throws IOException { ... } }The current duplicated approach is acceptable if you prefer explicit test isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/pragmatica/peg/perf/Phase2MarkResetChildrenParityTest.java` around lines 52 - 80, Extract the duplicated corpus/baseline logic into a shared helper (e.g., ParityTestSupport) and update the test to use it: move CORPUS_ROOT and BASELINE_ROOT constants plus the corpusFiles() Stream and a new loadBaseline(Path) method into ParityTestSupport, then change the MethodSource to refer to that shared corpusFiles and replace the baseline-reading/comparison code inside phase2MarkResetChildrenCstHashMatchesBaseline to call ParityTestSupport.loadBaseline(file) (and use ParityTestSupport.CORPUS_ROOT/BASELINE_ROOT as needed) so all nine parity tests share the same implementation.src/test/java/org/pragmatica/peg/perf/CstHash.java (1)
127-135:setAccessible(true)is unnecessary for public methods.Record accessor methods (like
span(),text(),rule(),children()) are public by definition. CallingsetAccessible(true)is redundant and may trigger warnings or reflection overhead on recent JVMs with strong encapsulation.♻️ Suggested simplification
private static Method findMethod(Class<?> cls, String name) { try { - var m = cls.getMethod(name); - m.setAccessible(true); - return m; + return cls.getMethod(name); } catch (NoSuchMethodException e) { throw new RuntimeException("No method " + name + "() on " + cls, e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/pragmatica/peg/perf/CstHash.java` around lines 127 - 135, The findMethod helper unnecessarily calls setAccessible(true) for public methods; remove the m.setAccessible(true) invocation from findMethod(Class<?> cls, String name) so it simply looks up and returns the public method (keeping the existing NoSuchMethodException handling) — this eliminates redundant reflection calls for record accessors like span(), text(), rule(), children().src/jmh/java/org/pragmatica/peg/bench/PackratStatsProbe.java (1)
71-84: Cache field access handles null correctly but could fail if field doesn't exist.The null check on line 78 handles the case where cache is empty, but
getDeclaredField("cache")will throwNoSuchFieldExceptionif a future generator change removes or renames the field. Consider wrapping in try-catch with a descriptive message.♻️ Defensive field access
- Field cacheField = parserClass.getDeclaredField("cache"); - cacheField.setAccessible(true); - `@SuppressWarnings`("unchecked") - var cache = (Map<Long, ?>) cacheField.get(instance); + Map<Long, ?> cache; + try { + Field cacheField = parserClass.getDeclaredField("cache"); + cacheField.setAccessible(true); + `@SuppressWarnings`("unchecked") + var cacheValue = (Map<Long, ?>) cacheField.get(instance); + cache = cacheValue; + } catch (NoSuchFieldException e) { + System.err.println("Warning: no 'cache' field found — packrat may be disabled"); + cache = Map.of(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmh/java/org/pragmatica/peg/bench/PackratStatsProbe.java` around lines 71 - 84, The reflection call parserClass.getDeclaredField("cache") can throw NoSuchFieldException if the generated parser no longer has a "cache" field; update PackratStatsProbe to defensively wrap the getDeclaredField/get access in a try-catch that catches NoSuchFieldException (and optionally IllegalAccessException), and handle it by logging or returning gracefully with a clear message (include parserClass and field name) rather than letting the exception propagate; ensure references to cacheField, cache, and the existing loop remain unchanged when the field is present.src/jmh/java/org/pragmatica/peg/bench/Java25ParseBenchmark.java (1)
160-187: Release the generated compiler artifacts after each trial.The
compileAndLoad()method creates a temp directory andURLClassLoaderper trial without cleanup. In repeated JMH trials, this accumulates generated source trees under the temp directory and keeps files pinned on some platforms due to open loaders. Store both as fields and add@TearDown(Level.Trial)to properly close the loader and delete the directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmh/java/org/pragmatica/peg/bench/Java25ParseBenchmark.java` around lines 160 - 187, The compileAndLoad method leaks resources by creating tempDir and URLClassLoader per trial without cleanup; modify the class so tempDir and classLoader returned/created by compileAndLoad are stored in fields (e.g., Path tempDir and URLClassLoader classLoader on the Java25ParseBenchmark class), change compileAndLoad to assign those fields instead of local vars, and add a `@TearDown`(Level.Trial) method (e.g., tearDownTrial) that closes classLoader and recursively deletes tempDir (handling exceptions) to release files and allow cleanup after each trial.src/main/java/org/pragmatica/peg/generator/ParserGenerator.java (1)
2321-2333: Unwindvisitingafter recursive reference analysis.Right now a successful walk leaves the rule name in the set, so a later sibling reference to the same non-recursive helper will be treated as recursion and disable the fast path unnecessarily.
Minimal fix
case Expression.Reference ref -> { if (!visiting.add(ref.ruleName())) yield false; // recursion — bail var target = grammar.rules() .stream() .filter(r -> r.name() .equals(ref.ruleName())) .findFirst(); - if (target.isEmpty()) yield false; - yield collectFirstChars(target.get() - .expression(), - out, - visiting); + if (target.isEmpty()) { + visiting.remove(ref.ruleName()); + yield false; + } + var ok = collectFirstChars(target.get() + .expression(), + out, + visiting); + visiting.remove(ref.ruleName()); + yield ok; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/pragmatica/peg/generator/ParserGenerator.java` around lines 2321 - 2333, The visiting set is not unwound after handling Expression.Reference, causing later sibling references to the same non-recursive rule to be treated as recursion; modify the branch that handles Expression.Reference (using ref.ruleName(), visiting, grammar.rules(), and collectFirstChars) to remove ref.ruleName() from visiting before yielding the result (ensure you add visiting.add(ref.ruleName()) early, call collectFirstChars(..., visiting), capture its boolean result, then remove ref.ruleName() from visiting and return that captured result).src/main/java/org/pragmatica/peg/generator/ChoiceDispatchAnalyzer.java (1)
159-165: Consider skipping leadingCuthere too.
Expression.Cutis zero-width, so sequences like^ 'foo'still have a fixed first consumed char. TreatingCutlike the predicate wrappers would let more choices use dispatch without changing semantics.Based on learnings: "Applies to src/main/java/org/pragmatica/peg/grammar/Expression.java : Use sealed interfaces for expression types (Expression.java) with implementations for Literal, CharClass, Any, Reference, Sequence, Choice, ZeroOrMore, OneOrMore, Optional, Repetition, And, Not, TokenBoundary, Ignore, Capture, BackReference, Cut, and Group"Small refactor
private static FirstChar firstLiteralInSequence(Expression.Sequence seq) { for (var el : seq.elements()) { - if (el instanceof Expression.And || el instanceof Expression.Not) { + if (el instanceof Expression.And || el instanceof Expression.Not || el instanceof Expression.Cut) { continue; } return firstLiteral(el); } return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/pragmatica/peg/generator/ChoiceDispatchAnalyzer.java` around lines 159 - 165, The method firstLiteralInSequence currently skips predicate wrappers but not zero-width Cut, so update firstLiteralInSequence to also treat Expression.Cut as a non-consuming element (i.e., continue past it) when iterating seq.elements(); specifically, add a check for "el instanceof Expression.Cut" alongside the existing "el instanceof Expression.And || el instanceof Expression.Not" so the method defers to firstLiteral(el) only for truly consuming elements and preserves semantics for sequences like Cut followed by a literal.src/test/resources/perf-corpus/format-examples/ChainAlignment.java (1)
134-152: Prefer fail-fast stubs overnullreturns in helper placeholders.If this fixture is accidentally executed,
nullcan obscure root cause and cause downstream NPEs. Using explicitUnsupportedOperationExceptionkeeps failures immediate and obvious.Proposed fail-fast stub update
Result<String> validate(String s) { - return null; + throw new UnsupportedOperationException("perf corpus fixture stub"); } @@ Result<Response> createResponse(Object tuple) { - return null; + throw new UnsupportedOperationException("perf corpus fixture stub"); } @@ Result<String> expensiveValidation(String s) { - return null; + throw new UnsupportedOperationException("perf corpus fixture stub"); } @@ String combine(A a, B b, C c) { - return null; + throw new UnsupportedOperationException("perf corpus fixture stub"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/resources/perf-corpus/format-examples/ChainAlignment.java` around lines 134 - 152, Replace the helper stubs that return null with fail-fast exceptions so accidental execution doesn't produce NPEs: in methods Result<String> validate(String), Result<Response> createResponse(Object), Result<String> expensiveValidation(String), String combine(A,B,C), and any logging helpers log(String) / logError(Object) where applicable, throw new UnsupportedOperationException (with a brief message identifying the method) instead of returning null; update those method bodies (e.g., validate, createResponse, expensiveValidation, combine, log, logError) to raise the exception so failures are immediate and descriptive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/PERF-REWORK-SPEC.md`:
- Around line 3-6: Update the status banner in PERF-REWORK-SPEC.md by replacing
the phrase "Not yet executed" with a release-accurate status (e.g., "Implemented
/ released" or "Shipped with verification artifacts") so the spec reflects that
the ParserGenerator rework and its verification artifacts are included in this
PR; edit the markdown line that currently reads "**Status:** proposal / spec for
an implementing agent. Not yet executed." to state the new status and optionally
add a brief parenthetical like "(shipped with this PR)" for clarity.
In `@src/main/java/org/pragmatica/peg/generator/ParserGenerator.java`:
- Around line 2305-2316: The Sequence branch in collectFirstChars incorrectly
stops at the first non-predicate element; change it to treat nullable prefixes
(e.g., Expression.Optional and Expression.ZeroOrMore) as potentially empty so
the walker continues to later elements when they can produce ε. Update
ParserGenerator.collectFirstChars: inside the case Expression.Sequence seq ->
loop, for each element skip predicates (Expression.And/Expression.Not) as
before, but if an element is nullable (create/use a helper like
isNullable(Expression) or reuse existing nullability logic for
Expression.Optional and Expression.ZeroOrMore) then still call collectFirstChars
on that element to accumulate starters and continue to the next element; only
return/stop early when you encounter a non-nullable element whose collected set
determines the result. Ensure Expression.OneOrMore is treated as non-nullable.
- Around line 4100-4112: The non-cached char-class branch currently returns
CstParseResult.failure("character class") making diagnostics differ from the
cached path; update the failure to use the same bracketed label used in
trackFailure so diagnostics are independent of charClassFailureCache — replace
the CstParseResult.failure("character class") calls in the else branch (the
block that uses isAtEnd(), startLoc, peek(), matchesPattern, negated,
trackFailure) so they return CstParseResult.failure("[" + (negated ? "^" : "") +
pattern + "]") while leaving trackFailure and matchesPattern unchanged.
In `@src/main/java/org/pragmatica/peg/parser/ParserConfig.java`:
- Around line 52-66: Add a compact record constructor to ParserConfig that
defensively copies the packratSkipRules parameter using
Set.copyOf(packratSkipRules) so the record always holds an immutable snapshot;
locate the record declaration ParserConfig and implement a compact constructor
that assigns all components by delegating to the canonical constructor while
replacing the incoming packratSkipRules with Set.copyOf(packratSkipRules) (and
handle null if needed) to prevent external mutations from affecting the record's
equality/cache-dependent behavior.
In `@src/test/java/org/pragmatica/peg/generator/GeneratorFlagInertnessTest.java`:
- Around line 24-61: The tests in GeneratorFlagInertnessTest (methods
generateCst_withDefaultConfig_isByteIdenticalToLegacyFactory,
generateCst_withDefaultConfigAndAdvancedReporting_isByteIdenticalToLegacyFactory,
generate_withDefaultConfig_isByteIdenticalToLegacyFactory) are only comparing
the no-arg ParserGenerator.create(...) overload to the explicit
ParserConfig.DEFAULT and so never exercise an actual "all flags off" config;
update each test to construct and pass an explicit all-off ParserConfig (e.g.,
build a ParserConfig with every perf flag set to false or use a dedicated
ParserConfig.ALL_OFF if available) when creating configuredSource so the
assertions compare legacySource to a true all-off configuration, or
alternatively rename the tests to state they verify overload parity rather than
flag inertness if you intend to keep the current behavior.
In `@src/test/java/org/pragmatica/peg/perf/GeneratedJava25Parser.java`:
- Around line 48-64: The publish ordering of parserClass and parseMethod is
racy: ensureLoaded currently assigns parserClass before parseMethod so another
thread can see parserClass != null and try to use a still-null parseMethod; to
fix, compute/compile the Class and obtain the Method into local variables (e.g.,
localParserClass and localParseMethod) inside the synchronized(LOCK) block and
only assign the shared parserClass (or a single holder field) as the last step
(or replace parserClass/parseMethod with one immutable holder object) so both
values become visible atomically; update ensureLoaded to set the shared
reference last and keep compileAndLoad(...) and parserClass/getMethod("parse",
String.class) lookups unchanged otherwise.
- Around line 146-167: The configKey(ParserConfig c) currently omits the
recoveryStrategy, causing different ParserConfig instances to collide; update
configKey to include a deterministic encoding of c.recoveryStrategy() (for
example append a separator and either the strategy name or its ordinal/string)
so that recoveryStrategy values produce distinct keys; locate the method
configKey(ParserConfig c) in GeneratedJava25Parser.java and append the
recoveryStrategy representation to the StringBuilder (using a stable,
filesystem-safe form) before returning the key.
In `@src/test/java/org/pragmatica/peg/perf/Phase1ParityTest.java`:
- Around line 49-52: The corpusFiles() helper silently returns an empty Stream
when CORPUS_ROOT is missing which yields false-green tests; change corpusFiles()
to fail fast by checking Files.isDirectory(CORPUS_ROOT) and throwing an
exception (e.g., throw new IOException("Missing corpus root: " + CORPUS_ROOT) or
an AssertionError) instead of returning Stream.empty(), and factor that check
into a shared helper (e.g., ensureCorpusRootExists or validateCorpusRoot) so
other phase parity tests can call it as well.
In `@src/test/resources/perf-corpus/format-examples/Imports.java`:
- Line 39: The declaration List<String> list = new ArrayList(); uses a raw
ArrayList; change the instantiation to use the diamond operator (List<String>
list = new ArrayList<>()) to remove unchecked-assignment warnings, or if the raw
type is intentional for testing, add an explicit comment next to the
instantiation (e.g., "// raw type intentional for parsing test") to document the
intent; update the occurrence of ArrayList() in Imports.java accordingly.
In `@src/test/resources/perf-corpus/format-examples/MultilineArguments.java`:
- Around line 105-107: The declaration ValidRequest currently defined as an
interface contains a constructor-like member which is invalid; change the
declaration kind from interface to a concrete type (e.g., convert the interface
ValidRequest to a record or class) and keep the parameter list (Object... args)
as the record/class constructor signature so the constructor body is valid;
update references to ValidRequest accordingly and remove the interface keyword
so the compiler accepts the constructor-like declaration.
In `@src/test/resources/perf-corpus/format-examples/MultilineParameters.java`:
- Around line 58-64: Replace the invalid interface declarations for ValidRequest
and Response with record declarations so the file compiles: change the interface
ValidRequest and interface Response blocks into records named ValidRequest and
Response that take an Object[] args component (e.g., record
ValidRequest(Object[] args) {} and record Response(Object[] args) {}), keeping
the empty bodies so the earlier new ValidRequest(...) and new Response(...)
instantiations are valid.
In `@src/test/resources/perf-corpus/format-examples/SwitchExpressions.java`:
- Around line 53-60: The switch expression in guardedSwitch contains duplicate
unguarded patterns (multiple "case String s" and "case Integer i") which are
illegal; update those cases to either add appropriate when-guards (e.g., case
String s when s.isEmpty(), case String s when s.length() < N, case Integer i
when i < 0, etc.) or merge/refactor them into a single case per type that
dispatches internally. Locate the switch inside guardedSwitch and apply guards
to each duplicate pattern or consolidate logic so each pattern type appears only
once.
---
Nitpick comments:
In `@docs/bench-results/java25-parse.json`:
- Around line 8-13: The benchmark snapshot currently commits a host-specific
absolute JVM path under the "jvm" key which makes the artifact non-portable;
change the snapshot generation (or post-processing) so that the "jvm" field is
normalized to a generic value (e.g., null, "", or a placeholder like
"JVM_PATH_OMITTED") or omitted entirely while retaining the actual version
details in "jdkVersion", "vmName", and "vmVersion"; update the code that writes
the JSON (the logic producing the "jvm" key) to substitute the sanitized value
instead of the absolute path and ensure tests or consumers expect the normalized
form.
In `@docs/bench-results/packrat-stats.txt`:
- Around line 1-2: The committed benchmark output contains machine-specific
compiler warnings at the top that add noise; remove the two preamble lines
("uses or overrides a deprecated API" and "Recompile with -Xlint:deprecation for
details") from docs/bench-results/packrat-stats.txt, or update the benchmark
generation workflow to strip such stderr/stdout warnings before committing
(i.e., filter these exact warning patterns from generated bench output so the
artifact is stable across machines).
In `@src/jmh/java/org/pragmatica/peg/bench/Java25ParseBenchmark.java`:
- Around line 160-187: The compileAndLoad method leaks resources by creating
tempDir and URLClassLoader per trial without cleanup; modify the class so
tempDir and classLoader returned/created by compileAndLoad are stored in fields
(e.g., Path tempDir and URLClassLoader classLoader on the Java25ParseBenchmark
class), change compileAndLoad to assign those fields instead of local vars, and
add a `@TearDown`(Level.Trial) method (e.g., tearDownTrial) that closes
classLoader and recursively deletes tempDir (handling exceptions) to release
files and allow cleanup after each trial.
In `@src/jmh/java/org/pragmatica/peg/bench/PackratStatsProbe.java`:
- Around line 71-84: The reflection call parserClass.getDeclaredField("cache")
can throw NoSuchFieldException if the generated parser no longer has a "cache"
field; update PackratStatsProbe to defensively wrap the getDeclaredField/get
access in a try-catch that catches NoSuchFieldException (and optionally
IllegalAccessException), and handle it by logging or returning gracefully with a
clear message (include parserClass and field name) rather than letting the
exception propagate; ensure references to cacheField, cache, and the existing
loop remain unchanged when the field is present.
In `@src/main/java/org/pragmatica/peg/generator/ChoiceDispatchAnalyzer.java`:
- Around line 159-165: The method firstLiteralInSequence currently skips
predicate wrappers but not zero-width Cut, so update firstLiteralInSequence to
also treat Expression.Cut as a non-consuming element (i.e., continue past it)
when iterating seq.elements(); specifically, add a check for "el instanceof
Expression.Cut" alongside the existing "el instanceof Expression.And || el
instanceof Expression.Not" so the method defers to firstLiteral(el) only for
truly consuming elements and preserves semantics for sequences like Cut followed
by a literal.
In `@src/main/java/org/pragmatica/peg/generator/ParserGenerator.java`:
- Around line 2321-2333: The visiting set is not unwound after handling
Expression.Reference, causing later sibling references to the same non-recursive
rule to be treated as recursion; modify the branch that handles
Expression.Reference (using ref.ruleName(), visiting, grammar.rules(), and
collectFirstChars) to remove ref.ruleName() from visiting before yielding the
result (ensure you add visiting.add(ref.ruleName()) early, call
collectFirstChars(..., visiting), capture its boolean result, then remove
ref.ruleName() from visiting and return that captured result).
In `@src/test/java/org/pragmatica/peg/perf/CstHash.java`:
- Around line 127-135: The findMethod helper unnecessarily calls
setAccessible(true) for public methods; remove the m.setAccessible(true)
invocation from findMethod(Class<?> cls, String name) so it simply looks up and
returns the public method (keeping the existing NoSuchMethodException handling)
— this eliminates redundant reflection calls for record accessors like span(),
text(), rule(), children().
In `@src/test/java/org/pragmatica/peg/perf/CstReconstruct.java`:
- Around line 85-93: The helper findMethod(Class<?> cls, String name) calls
m.setAccessible(true) unnecessarily for public record accessor methods; remove
the m.setAccessible(true) call and simply return the Method obtained from
cls.getMethod(name), keeping the existing try/catch and RuntimeException
behavior for NoSuchMethodException so tests still fail cleanly when the accessor
is missing.
In
`@src/test/java/org/pragmatica/peg/perf/Phase2MarkResetChildrenParityTest.java`:
- Around line 52-80: Extract the duplicated corpus/baseline logic into a shared
helper (e.g., ParityTestSupport) and update the test to use it: move CORPUS_ROOT
and BASELINE_ROOT constants plus the corpusFiles() Stream and a new
loadBaseline(Path) method into ParityTestSupport, then change the MethodSource
to refer to that shared corpusFiles and replace the baseline-reading/comparison
code inside phase2MarkResetChildrenCstHashMatchesBaseline to call
ParityTestSupport.loadBaseline(file) (and use
ParityTestSupport.CORPUS_ROOT/BASELINE_ROOT as needed) so all nine parity tests
share the same implementation.
In `@src/test/resources/perf-corpus/flow-format-examples/BlankLineRules.java`:
- Line 26: The record declaration for Inner lacks a space before the opening
brace; update the declaration of record Inner(String value) {} (symbol: Inner)
to include a space before the '{' so it follows standard Java formatting
conventions (i.e., change "record Inner(String value){}" to "record Inner(String
value) {}").
- Line 6: The private field `active` in class `BlankLineRules` is never used
outside the constructor; either remove the unused field or add a public accessor
if it should be exposed—locate the `private final boolean active;` declaration
and the constructor that initializes it, then either delete the field and
related constructor parameter or implement a getter method (e.g., `isActive()`
or `getActive()`) so callers can read the value.
In `@src/test/resources/perf-corpus/format-examples/ChainAlignment.java`:
- Around line 134-152: Replace the helper stubs that return null with fail-fast
exceptions so accidental execution doesn't produce NPEs: in methods
Result<String> validate(String), Result<Response> createResponse(Object),
Result<String> expensiveValidation(String), String combine(A,B,C), and any
logging helpers log(String) / logError(Object) where applicable, throw new
UnsupportedOperationException (with a brief message identifying the method)
instead of returning null; update those method bodies (e.g., validate,
createResponse, expensiveValidation, combine, log, logError) to raise the
exception so failures are immediate and descriptive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae75d478-a183-4511-ae3e-4ac0e9474af3
📒 Files selected for processing (100)
CHANGELOG.mdCLAUDE.mdREADME.mddocs/PERF-REWORK-SPEC.mddocs/bench-results/java25-parse.jsondocs/bench-results/packrat-stats.txtpom.xmlsrc/jmh/java/org/pragmatica/peg/bench/Java25ParseBenchmark.javasrc/jmh/java/org/pragmatica/peg/bench/PackratStatsProbe.javasrc/main/java/org/pragmatica/peg/PegParser.javasrc/main/java/org/pragmatica/peg/generator/ChoiceDispatchAnalyzer.javasrc/main/java/org/pragmatica/peg/generator/ParserGenerator.javasrc/main/java/org/pragmatica/peg/parser/ParserConfig.javasrc/test/java/org/pragmatica/peg/examples/Java25GrammarExample.javasrc/test/java/org/pragmatica/peg/generator/ChoiceDispatchAnalyzerTest.javasrc/test/java/org/pragmatica/peg/generator/GeneratorFlagInertnessTest.javasrc/test/java/org/pragmatica/peg/parser/ParsingContextTest.javasrc/test/java/org/pragmatica/peg/perf/BaselineGenerator.javasrc/test/java/org/pragmatica/peg/perf/BaselineGeneratorRunner.javasrc/test/java/org/pragmatica/peg/perf/CorpusParityTest.javasrc/test/java/org/pragmatica/peg/perf/CstHash.javasrc/test/java/org/pragmatica/peg/perf/CstReconstruct.javasrc/test/java/org/pragmatica/peg/perf/GeneratedJava25Parser.javasrc/test/java/org/pragmatica/peg/perf/Phase1ParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2AllStructuralParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2ChoiceDispatchAndMarkResetParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2ChoiceDispatchParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2InlineLocationsParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2MarkResetChildrenParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2SelectivePackratEmptySetParityTest.javasrc/test/java/org/pragmatica/peg/perf/Phase2SelectivePackratParityTest.javasrc/test/java/org/pragmatica/peg/perf/RoundTripTest.javasrc/test/resources/java25.pegsrc/test/resources/perf-corpus-baseline/flow-format-examples/BlankLineRules.java.hashsrc/test/resources/perf-corpus-baseline/flow-format-examples/BlankLineRules.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/Annotations.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/Annotations.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/BlankLines.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/BlankLines.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/ChainAlignment.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/ChainAlignment.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/ClassLiterals.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/ClassLiterals.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/Comments.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/Comments.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/CompoundAssignments.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/CompoundAssignments.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/DeepGenerics.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/DeepGenerics.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/Enums.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/Enums.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/ExhaustiveSwitchPatterns.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/ExhaustiveSwitchPatterns.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/Imports.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/Imports.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/KeywordPrefixedIdentifiers.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/KeywordPrefixedIdentifiers.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/Lambdas.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/Lambdas.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/LineWrapping.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/LineWrapping.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/ModuleInfo.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/ModuleInfo.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/MultilineArguments.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/MultilineArguments.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/MultilineParameters.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/MultilineParameters.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/Records.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/Records.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/SwitchExpressions.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/SwitchExpressions.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/TernaryOperators.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/TernaryOperators.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/format-examples/TextBlocks.java.hashsrc/test/resources/perf-corpus-baseline/format-examples/TextBlocks.java.ruleHits.txtsrc/test/resources/perf-corpus-baseline/large/FactoryClassGenerator.java.txt.hashsrc/test/resources/perf-corpus-baseline/large/FactoryClassGenerator.java.txt.ruleHits.txtsrc/test/resources/perf-corpus-baseline/ruleCoverage.txtsrc/test/resources/perf-corpus/flow-format-examples/BlankLineRules.javasrc/test/resources/perf-corpus/format-examples/Annotations.javasrc/test/resources/perf-corpus/format-examples/BlankLines.javasrc/test/resources/perf-corpus/format-examples/ChainAlignment.javasrc/test/resources/perf-corpus/format-examples/ClassLiterals.javasrc/test/resources/perf-corpus/format-examples/Comments.javasrc/test/resources/perf-corpus/format-examples/CompoundAssignments.javasrc/test/resources/perf-corpus/format-examples/DeepGenerics.javasrc/test/resources/perf-corpus/format-examples/Enums.javasrc/test/resources/perf-corpus/format-examples/ExhaustiveSwitchPatterns.javasrc/test/resources/perf-corpus/format-examples/Imports.javasrc/test/resources/perf-corpus/format-examples/KeywordPrefixedIdentifiers.javasrc/test/resources/perf-corpus/format-examples/Lambdas.javasrc/test/resources/perf-corpus/format-examples/LineWrapping.javasrc/test/resources/perf-corpus/format-examples/ModuleInfo.javasrc/test/resources/perf-corpus/format-examples/MultilineArguments.javasrc/test/resources/perf-corpus/format-examples/MultilineParameters.javasrc/test/resources/perf-corpus/format-examples/Records.javasrc/test/resources/perf-corpus/format-examples/SwitchExpressions.javasrc/test/resources/perf-corpus/format-examples/TernaryOperators.javasrc/test/resources/perf-corpus/format-examples/TextBlocks.javasrc/test/resources/perf-corpus/large/FactoryClassGenerator.java.txt
Comment on lines
+3
to
+6
| **Target repo:** `java-peglib` (self-contained; no dependency on any other repo). | ||
| **Status:** proposal / spec for an implementing agent. Not yet executed. | ||
| **Scope:** optimizations to the PEG code generator (`ParserGenerator`) only. | ||
| `PegEngine` (the interpreter) is **out of scope** for this pass. |
There was a problem hiding this comment.
Update the status banner to match the release state.
Line 4 still says this work is “Not yet executed”, but this PR already ships the rework and its verification artifacts. Leaving that in place will make the spec look stale immediately after release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/PERF-REWORK-SPEC.md` around lines 3 - 6, Update the status banner in
PERF-REWORK-SPEC.md by replacing the phrase "Not yet executed" with a
release-accurate status (e.g., "Implemented / released" or "Shipped with
verification artifacts") so the spec reflects that the ParserGenerator rework
and its verification artifacts are included in this PR; edit the markdown line
that currently reads "**Status:** proposal / spec for an implementing agent. Not
yet executed." to state the new status and optionally add a brief parenthetical
like "(shipped with this PR)" for clarity.
Comment on lines
+2305
to
+2316
| case Expression.Sequence seq -> { | ||
| for (var el : seq.elements()) { | ||
| // skip over leading predicates — they don't consume | ||
| if (el instanceof Expression.And || el instanceof Expression.Not) continue; | ||
| yield collectFirstChars(el, out, visiting); | ||
| } | ||
| yield false; | ||
| } | ||
| case Expression.Group grp -> collectFirstChars(grp.expression(), out, visiting); | ||
| case Expression.ZeroOrMore zom -> collectFirstChars(zom.expression(), out, visiting); | ||
| case Expression.OneOrMore oom -> collectFirstChars(oom.expression(), out, visiting); | ||
| case Expression.Optional opt -> collectFirstChars(opt.expression(), out, visiting); |
There was a problem hiding this comment.
Don't treat nullable prefixes as fixed starters.
This sequence walker stops at the first non-predicate element, but Optional/ZeroOrMore prefixes can yield ε. For whitespace shapes like spaces* comment, the derived set misses '/', so the fast path can incorrectly return early and skip valid trivia.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/pragmatica/peg/generator/ParserGenerator.java` around lines
2305 - 2316, The Sequence branch in collectFirstChars incorrectly stops at the
first non-predicate element; change it to treat nullable prefixes (e.g.,
Expression.Optional and Expression.ZeroOrMore) as potentially empty so the
walker continues to later elements when they can produce ε. Update
ParserGenerator.collectFirstChars: inside the case Expression.Sequence seq ->
loop, for each element skip predicates (Expression.And/Expression.Not) as
before, but if an element is nullable (create/use a helper like
isNullable(Expression) or reuse existing nullability logic for
Expression.Optional and Expression.ZeroOrMore) then still call collectFirstChars
on that element to accumulate starters and continue to the next element; only
return/stop early when you encounter a non-nullable element whose collected set
determines the result. Ensure Expression.OneOrMore is treated as non-nullable.
Comment on lines
+4100
to
+4112
| }else { | ||
| sb.append(" if (isAtEnd()) {\n"); | ||
| sb.append(" trackFailure(\"[\" + (negated ? \"^\" : \"\") + pattern + \"]\");\n"); | ||
| sb.append(" return CstParseResult.failure(\"character class\");\n"); | ||
| sb.append(" }\n"); | ||
| sb.append(" var startLoc = location();\n"); | ||
| sb.append(" char c = peek();\n"); | ||
| sb.append(" boolean matches = matchesPattern(c, pattern, caseInsensitive);\n"); | ||
| sb.append(" if (negated) matches = !matches;\n"); | ||
| sb.append(" if (!matches) {\n"); | ||
| sb.append(" trackFailure(\"[\" + (negated ? \"^\" : \"\") + pattern + \"]\");\n"); | ||
| sb.append(" return CstParseResult.failure(\"character class\");\n"); | ||
| sb.append(" }\n"); |
There was a problem hiding this comment.
Keep char-class failure text independent of the cache flag.
The cached branch returns the bracketed label, but this path still returns "character class". That makes diagnostics depend on charClassFailureCache, which should be a perf-only toggle.
Suggested fix
} else {
sb.append(" if (isAtEnd()) {\n");
- sb.append(" trackFailure(\"[\" + (negated ? \"^\" : \"\") + pattern + \"]\");\n");
- sb.append(" return CstParseResult.failure(\"character class\");\n");
+ sb.append(" var expected = \"[\" + (negated ? \"^\" : \"\") + pattern + \"]\";\n");
+ sb.append(" trackFailure(expected);\n");
+ sb.append(" return CstParseResult.failure(expected);\n");
sb.append(" }\n");
sb.append(" var startLoc = location();\n");
sb.append(" char c = peek();\n");
sb.append(" boolean matches = matchesPattern(c, pattern, caseInsensitive);\n");
sb.append(" if (negated) matches = !matches;\n");
sb.append(" if (!matches) {\n");
- sb.append(" trackFailure(\"[\" + (negated ? \"^\" : \"\") + pattern + \"]\");\n");
- sb.append(" return CstParseResult.failure(\"character class\");\n");
+ sb.append(" var expected = \"[\" + (negated ? \"^\" : \"\") + pattern + \"]\";\n");
+ sb.append(" trackFailure(expected);\n");
+ sb.append(" return CstParseResult.failure(expected);\n");
sb.append(" }\n");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| }else { | |
| sb.append(" if (isAtEnd()) {\n"); | |
| sb.append(" trackFailure(\"[\" + (negated ? \"^\" : \"\") + pattern + \"]\");\n"); | |
| sb.append(" return CstParseResult.failure(\"character class\");\n"); | |
| sb.append(" }\n"); | |
| sb.append(" var startLoc = location();\n"); | |
| sb.append(" char c = peek();\n"); | |
| sb.append(" boolean matches = matchesPattern(c, pattern, caseInsensitive);\n"); | |
| sb.append(" if (negated) matches = !matches;\n"); | |
| sb.append(" if (!matches) {\n"); | |
| sb.append(" trackFailure(\"[\" + (negated ? \"^\" : \"\") + pattern + \"]\");\n"); | |
| sb.append(" return CstParseResult.failure(\"character class\");\n"); | |
| sb.append(" }\n"); | |
| }else { | |
| sb.append(" if (isAtEnd()) {\n"); | |
| sb.append(" var expected = \"[\" + (negated ? \"^\" : \"\") + pattern + \"]\";\n"); | |
| sb.append(" trackFailure(expected);\n"); | |
| sb.append(" return CstParseResult.failure(expected);\n"); | |
| sb.append(" }\n"); | |
| sb.append(" var startLoc = location();\n"); | |
| sb.append(" char c = peek();\n"); | |
| sb.append(" boolean matches = matchesPattern(c, pattern, caseInsensitive);\n"); | |
| sb.append(" if (negated) matches = !matches;\n"); | |
| sb.append(" if (!matches) {\n"); | |
| sb.append(" var expected = \"[\" + (negated ? \"^\" : \"\") + pattern + \"]\";\n"); | |
| sb.append(" trackFailure(expected);\n"); | |
| sb.append(" return CstParseResult.failure(expected);\n"); | |
| sb.append(" }\n"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/pragmatica/peg/generator/ParserGenerator.java` around lines
4100 - 4112, The non-cached char-class branch currently returns
CstParseResult.failure("character class") making diagnostics differ from the
cached path; update the failure to use the same bracketed label used in
trackFailure so diagnostics are independent of charClassFailureCache — replace
the CstParseResult.failure("character class") calls in the else branch (the
block that uses isAtEnd(), startLoc, peek(), matchesPattern, negated,
trackFailure) so they return CstParseResult.failure("[" + (negated ? "^" : "") +
pattern + "]") while leaving trackFailure and matchesPattern unchanged.
Comment on lines
52
to
+66
| public record ParserConfig( | ||
| boolean packratEnabled, | ||
| RecoveryStrategy recoveryStrategy, | ||
| boolean captureTrivia) { | ||
| boolean captureTrivia, | ||
| boolean fastTrackFailure, | ||
| boolean literalFailureCache, | ||
| boolean charClassFailureCache, | ||
| boolean bulkAdvanceLiteral, | ||
| boolean skipWhitespaceFastPath, | ||
| boolean reuseEndLocation, | ||
| boolean choiceDispatch, | ||
| boolean markResetChildren, | ||
| boolean inlineLocations, | ||
| boolean selectivePackrat, | ||
| Set<String> packratSkipRules) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the ParserConfig.java file
find . -name "ParserConfig.java" -type fRepository: siy/java-peglib
Length of output: 118
🏁 Script executed:
# Read the ParserConfig.java file
cat -n ./src/main/java/org/pragmatica/peg/parser/ParserConfig.javaRepository: siy/java-peglib
Length of output: 4901
🏁 Script executed:
# Search for usages of ParserConfig to understand how it's being used
rg "new ParserConfig" -A 5 -B 2Repository: siy/java-peglib
Length of output: 12779
🏁 Script executed:
# Search for cases where packratSkipRules is being passed - check if any mutable sets are used
rg "packratSkipRules|ParserConfig\(" -B 5 -A 10 | grep -A 20 "packratSkipRules"Repository: siy/java-peglib
Length of output: 38873
Add a compact record constructor to defensively copy packratSkipRules.
The record currently documents that packratSkipRules must be treated as immutable by callers and places the burden on them to wrap mutable sources with Set.copyOf(...). However, the constructor does not enforce this. If a caller passes a mutable set and later mutates it externally, any code generation or caching logic that depends on the config's equality or contents becomes unstable. A compact constructor using Set.copyOf(...) shifts the defensive copy to the record itself, eliminating caller error surface.
Suggested fix
public record ParserConfig(
boolean packratEnabled,
RecoveryStrategy recoveryStrategy,
boolean captureTrivia,
boolean fastTrackFailure,
boolean literalFailureCache,
boolean charClassFailureCache,
boolean bulkAdvanceLiteral,
boolean skipWhitespaceFastPath,
boolean reuseEndLocation,
boolean choiceDispatch,
boolean markResetChildren,
boolean inlineLocations,
boolean selectivePackrat,
Set<String> packratSkipRules) {
+ public ParserConfig {
+ packratSkipRules = Set.copyOf(packratSkipRules);
+ }
+
public static final ParserConfig DEFAULT = new ParserConfig(
true, RecoveryStrategy.BASIC, true, true, true, true, true, true, true, true, false, false, false, Set.of());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public record ParserConfig( | |
| boolean packratEnabled, | |
| RecoveryStrategy recoveryStrategy, | |
| boolean captureTrivia) { | |
| boolean captureTrivia, | |
| boolean fastTrackFailure, | |
| boolean literalFailureCache, | |
| boolean charClassFailureCache, | |
| boolean bulkAdvanceLiteral, | |
| boolean skipWhitespaceFastPath, | |
| boolean reuseEndLocation, | |
| boolean choiceDispatch, | |
| boolean markResetChildren, | |
| boolean inlineLocations, | |
| boolean selectivePackrat, | |
| Set<String> packratSkipRules) { | |
| public record ParserConfig( | |
| boolean packratEnabled, | |
| RecoveryStrategy recoveryStrategy, | |
| boolean captureTrivia, | |
| boolean fastTrackFailure, | |
| boolean literalFailureCache, | |
| boolean charClassFailureCache, | |
| boolean bulkAdvanceLiteral, | |
| boolean skipWhitespaceFastPath, | |
| boolean reuseEndLocation, | |
| boolean choiceDispatch, | |
| boolean markResetChildren, | |
| boolean inlineLocations, | |
| boolean selectivePackrat, | |
| Set<String> packratSkipRules) { | |
| public ParserConfig { | |
| packratSkipRules = Set.copyOf(packratSkipRules); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/pragmatica/peg/parser/ParserConfig.java` around lines 52 -
66, Add a compact record constructor to ParserConfig that defensively copies the
packratSkipRules parameter using Set.copyOf(packratSkipRules) so the record
always holds an immutable snapshot; locate the record declaration ParserConfig
and implement a compact constructor that assigns all components by delegating to
the canonical constructor while replacing the incoming packratSkipRules with
Set.copyOf(packratSkipRules) (and handle null if needed) to prevent external
mutations from affecting the record's equality/cache-dependent behavior.
Comment on lines
+24
to
+61
| @Test | ||
| void generateCst_withDefaultConfig_isByteIdenticalToLegacyFactory() { | ||
| var grammar = loadJava25Grammar(); | ||
|
|
||
| var legacySource = ParserGenerator.create(grammar, "gen", "Java25Parser") | ||
| .generateCst(); | ||
| var configuredSource = ParserGenerator.create(grammar, "gen", "Java25Parser", ParserConfig.DEFAULT) | ||
| .generateCst(); | ||
|
|
||
| assertEquals(legacySource, configuredSource, | ||
| "Generator output must be byte-identical when all perf flags are off"); | ||
| } | ||
|
|
||
| @Test | ||
| void generateCst_withDefaultConfigAndAdvancedReporting_isByteIdenticalToLegacyFactory() { | ||
| var grammar = loadJava25Grammar(); | ||
|
|
||
| var legacySource = ParserGenerator.create(grammar, "gen", "Java25Parser", ErrorReporting.ADVANCED) | ||
| .generateCst(); | ||
| var configuredSource = ParserGenerator.create(grammar, "gen", "Java25Parser", | ||
| ErrorReporting.ADVANCED, ParserConfig.DEFAULT) | ||
| .generateCst(); | ||
|
|
||
| assertEquals(legacySource, configuredSource, | ||
| "Generator output must be byte-identical for ADVANCED reporting when all perf flags are off"); | ||
| } | ||
|
|
||
| @Test | ||
| void generate_withDefaultConfig_isByteIdenticalToLegacyFactory() { | ||
| var grammar = loadJava25Grammar(); | ||
|
|
||
| var legacySource = ParserGenerator.create(grammar, "gen", "Java25Parser") | ||
| .generate(); | ||
| var configuredSource = ParserGenerator.create(grammar, "gen", "Java25Parser", ParserConfig.DEFAULT) | ||
| .generate(); | ||
|
|
||
| assertEquals(legacySource, configuredSource, | ||
| "Action-bearing generator output must be byte-identical when all perf flags are off"); |
There was a problem hiding this comment.
This test never exercises the “flags off” invariant.
Per src/main/java/org/pragmatica/peg/generator/ParserGenerator.java:114-138, the no-config create(...) overload already delegates to ParserConfig.DEFAULT, and the configured side passes that same config explicitly. So these assertions only prove the overloads are wired identically; they cannot catch regressions when generator-time flags are actually off, which is what the class name/Javadoc claim. Use an explicit all-off config plus a golden/pre-change output, or rename this to overload-parity if that is the real contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/pragmatica/peg/generator/GeneratorFlagInertnessTest.java`
around lines 24 - 61, The tests in GeneratorFlagInertnessTest (methods
generateCst_withDefaultConfig_isByteIdenticalToLegacyFactory,
generateCst_withDefaultConfigAndAdvancedReporting_isByteIdenticalToLegacyFactory,
generate_withDefaultConfig_isByteIdenticalToLegacyFactory) are only comparing
the no-arg ParserGenerator.create(...) overload to the explicit
ParserConfig.DEFAULT and so never exercise an actual "all flags off" config;
update each test to construct and pass an explicit all-off ParserConfig (e.g.,
build a ParserConfig with every perf flag set to false or use a dedicated
ParserConfig.ALL_OFF if available) when creating configuredSource so the
assertions compare legacySource to a true all-off configuration, or
alternatively rename the tests to state they verify overload parity rather than
flag inertness if you intend to keep the current behavior.
Comment on lines
+49
to
+52
| static Stream<Path> corpusFiles() throws IOException { | ||
| if (!Files.isDirectory(CORPUS_ROOT)) { | ||
| return Stream.empty(); | ||
| } |
There was a problem hiding this comment.
Don't silently skip the corpus.
If CORPUS_ROOT is missing, this returns zero arguments and the entire parity class passes without exercising anything. For a regression suite, that is a false green; fail fast on a missing corpus root instead, and ideally share that guard with the other phase parity tests too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/pragmatica/peg/perf/Phase1ParityTest.java` around lines 49
- 52, The corpusFiles() helper silently returns an empty Stream when CORPUS_ROOT
is missing which yields false-green tests; change corpusFiles() to fail fast by
checking Files.isDirectory(CORPUS_ROOT) and throwing an exception (e.g., throw
new IOException("Missing corpus root: " + CORPUS_ROOT) or an AssertionError)
instead of returning Stream.empty(), and factor that check into a shared helper
(e.g., ensureCorpusRootExists or validateCorpusRoot) so other phase parity tests
can call it as well.
| Promise<String> promise = Promise.success("test"); | ||
| Result<String> result = success("test"); | ||
| Unit u = unit(); | ||
| List<String> list = new ArrayList(); |
There was a problem hiding this comment.
Raw type usage: consider using diamond operator.
new ArrayList() is a raw type. For Java 7+ code, use new ArrayList<>() to avoid unchecked assignment warnings. If this is intentional to test raw type parsing, consider adding a comment to clarify.
💡 Suggested fix
- List<String> list = new ArrayList();
+ List<String> list = new ArrayList<>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<String> list = new ArrayList(); | |
| List<String> list = new ArrayList<>(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/perf-corpus/format-examples/Imports.java` at line 39, The
declaration List<String> list = new ArrayList(); uses a raw ArrayList; change
the instantiation to use the diamond operator (List<String> list = new
ArrayList<>()) to remove unchecked-assignment warnings, or if the raw type is
intentional for testing, add an explicit comment next to the instantiation
(e.g., "// raw type intentional for parsing test") to document the intent;
update the occurrence of ArrayList() in Imports.java accordingly.
Comment on lines
+105
to
+107
| interface ValidRequest { | ||
| ValidRequest(Object... args) {} | ||
| } |
There was a problem hiding this comment.
Invalid Java syntax: interfaces cannot have constructors.
The ValidRequest interface declares what looks like a constructor with a body. Interfaces cannot have constructors in Java. This should be a record or class instead.
💡 Suggested fix - use a record
- interface ValidRequest {
- ValidRequest(Object... args) {}
- }
+ record ValidRequest(Object... args) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| interface ValidRequest { | |
| ValidRequest(Object... args) {} | |
| } | |
| record ValidRequest(Object... args) {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/perf-corpus/format-examples/MultilineArguments.java`
around lines 105 - 107, The declaration ValidRequest currently defined as an
interface contains a constructor-like member which is invalid; change the
declaration kind from interface to a concrete type (e.g., convert the interface
ValidRequest to a record or class) and keep the parameter list (Object... args)
as the record/class constructor signature so the constructor body is valid;
update references to ValidRequest accordingly and remove the interface keyword
so the compiler accepts the constructor-like declaration.
Comment on lines
+58
to
+64
| interface ValidRequest { | ||
| ValidRequest(Object... args) {} | ||
| } | ||
|
|
||
| interface Response { | ||
| Response(Object... args) {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Are constructors allowed in Java interfaces, and can an interface be instantiated directly with new InterfaceName(...) in Java 25?
💡 Result:
No, constructors are not allowed in Java interfaces, and interfaces cannot be instantiated directly with new InterfaceName(...) in Java 25. Interfaces in Java are abstract reference types that define contracts via abstract methods, default methods, static methods, and constants, but they explicitly do not support constructors. The Java Language Specification (JLS) Chapter 9 on Interfaces describes interface declarations and members (fields, methods, nested types), with no provision for constructors in the grammar or semantics. Interface members are limited to constant declarations, abstract methods, default methods, static methods, and nested types; constructors are absent. Attempting to declare a constructor in an interface results in a compile-time error. Interfaces cannot be instantiated directly because they are implicitly abstract. Direct instantiation like new InterfaceName(...) is invalid and produces a compile-time error: "InterfaceName is abstract; cannot be instantiated." Objects compatible with an interface type must be instances of concrete classes that implement the interface, anonymous classes, or lambda expressions (for functional interfaces). No changes in Java 25 alter this fundamental behavior. JDK 25 release notes and JEPs (e.g., JEP 513 on flexible constructor bodies) address class constructors only, with no mention of interfaces gaining constructors or direct instantiation capability. This holds across JLS versions from SE 6 to SE 24, confirming consistency up to Java 25.
Citations:
- 1: https://docs.oracle.com/javase/specs/jls/se24/html/jls-9.html
- 2: https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html
- 3: https://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html
- 4: https://docs.oracle.com/javase/specs/jls/se6/html/interfaces.html
- 5: https://www.javathinking.com/blog/can-i-create-an-object-for-interface/
- 6: https://codingtechroom.com/question/instantiate-interface-java
- 7: https://stackoverflow.com/questions/16750772/instantiating-interfaces-in-java
- 8: https://docs.oracle.com/javase/specs/jls/se19/html/jls-9.html
- 9: https://stackoverflow.com/questions/40813160/do-interfaces-have-a-constructor-in-java
- 10: https://stackoverflow.com/questions/2804041/constructor-in-an-interface
Replace interface declarations with records to ensure corpus contains valid Java.
Interfaces cannot declare constructors, and the new ValidRequest(...) and new Response(...) instantiations on earlier lines are invalid. As a perf-corpus fixture, this code should represent compilable Java; replace with record types instead.
Suggested shape
- interface ValidRequest {
- ValidRequest(Object... args) {}
- }
-
- interface Response {
- Response(Object... args) {}
- }
+ record ValidRequest(Email email, Password password, Option<ReferralCode> referral) {}
+
+ record Response(String userId,
+ String token,
+ long expiresAt,
+ boolean active,
+ Option<String> refreshToken) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/perf-corpus/format-examples/MultilineParameters.java`
around lines 58 - 64, Replace the invalid interface declarations for
ValidRequest and Response with record declarations so the file compiles: change
the interface ValidRequest and interface Response blocks into records named
ValidRequest and Response that take an Object[] args component (e.g., record
ValidRequest(Object[] args) {} and record Response(Object[] args) {}), keeping
the empty bodies so the earlier new ValidRequest(...) and new Response(...)
instantiations are valid.
Comment on lines
+53
to
+60
| String guardedSwitch(Object obj) { | ||
| return switch (obj){ | ||
| case String s -> "empty string"; | ||
| case String s -> "short string: " + s; | ||
| case String s -> "long string: " + s; | ||
| case Integer i -> "negative: " + i; | ||
| case Integer i -> "non-negative: " + i; | ||
| default -> "other"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java 25 switch pattern matching, are later case String s ->orcase Integer i ->labels allowed after an earlier unguarded case of the same type, or are they dominated unless guarded withwhen?
💡 Result:
No, later case String s -> or case Integer i -> labels (unguarded) are not allowed after an earlier unguarded case of the same type; they are dominated and cause a compile-time error. They are only allowed if guarded with when, as guarded patterns are not checked for dominance against other patterns (though ordering is still recommended for runtime behavior).
Citations:
- 1: https://docs.oracle.com/en/java/javase/25/language/pattern-matching-switch.html
- 2: https://openjdk.org/jeps/441
- 3: https://docs.oracle.com/en/java/javase/24/language/pattern-matching-switch.html
- 4: https://openjdk.java.net/jeps/441
- 5: https://cr.openjdk.org/~gbierman/jep440+441/jep440+441-20230406/specs/patterns-switch-record-patterns-jls.html
Add when guards or refactor to avoid duplicate pattern types.
The code contains unreachable dominated patterns. In Java 25, later case String s -> labels cannot follow an earlier unguarded case String s -> (same applies to Integer i). These cases cause a compile-time error unless guarded with when.
Suggested fix
- case String s -> "empty string";
- case String s -> "short string: " + s;
- case String s -> "long string: " + s;
- case Integer i -> "negative: " + i;
- case Integer i -> "non-negative: " + i;
+ case String s when s.isEmpty() -> "empty string";
+ case String s when s.length() < 5 -> "short string: " + s;
+ case String s -> "long string: " + s;
+ case Integer i when i < 0 -> "negative: " + i;
+ case Integer i -> "non-negative: " + i;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/perf-corpus/format-examples/SwitchExpressions.java` around
lines 53 - 60, The switch expression in guardedSwitch contains duplicate
unguarded patterns (multiple "case String s" and "case Integer i") which are
illegal; update those cases to either add appropriate when-guards (e.g., case
String s when s.isEmpty(), case String s when s.length() < N, case Integer i
when i < 0, etc.) or merge/refactor them into a single case per type that
dispatches internally. Locate the switch inside guardedSwitch and apply guards
to each duplicate pattern or consolidate logic so each pattern type appears only
once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Performance rework for the generated CST parser. Landing the spec at
docs/PERF-REWORK-SPEC.md.Measured on the 1,900-LOC
FactoryClassGenerator.java.txtfixture (JMH avgT, JDK 25.0.2):choiceDispatch): 100.7 ms/op — 4.23× speedup (exceeds the spec's 1.5× bar and the 2× stretch)What changed
fastTrackFailure,literalFailureCache,charClassFailureCache,bulkAdvanceLiteral,skipWhitespaceFastPath,reuseEndLocation— hand-edits ported to the generator's emitter templates per spec §6. Combined measured win: 1.70× vs baseline.choiceDispatch→ default on (2.49× over phase-1 baseline).markResetChildren,inlineLocations,selectivePackrat→ default off — no statistically significant individual win on the reference JVM.matchCharClassCstfailure string is now the bracketed label ("[a-z]") instead of the generic"character class". Zero breakage in existing tests.Verification
GeneratorFlagInertnessTestasserts byte-identical generator output when flags are off.RoundTripTest— documents a pre-existing intra-sequence trivia-drop gap).docs/bench-results/.Test plan
mvn test— 565/565 + 1 skippedmvn install— artifact published to local~/.m2mvn -Pbench -DskipTests package && java -jar target/benchmarks.jar— JMH matrix completesSummary by CodeRabbit
New Features
ParserConfigflags (phase-1: failure tracking, caching, whitespace fast-path; phase-2: choice dispatch, location inlining, selective packrat).Performance
Tests
Documentation
Chores