Skip to content

build: add reproducible lookahead auditing - #2829

Open
fudianchn wants to merge 1 commit into
JSQLParser:masterfrom
fudianchn:perf/lookahead-audit
Open

fudianchn wants to merge 1 commit into
JSQLParser:masterfrom
fudianchn:perf/lookahead-audit

Conversation

@fudianchn

@fudianchn fudianchn commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

AI disclosure: this change was prepared with AI coding agents. This revision is awaiting my line-by-line review.

What

Bound 57 complete single-token syntactic probes to depth one, remove one redundant DBMSDestination guard, and add cached lookahead analysis, an inventory and explicit review-evidence scopes.

Why

Follow the maintainer's request for a systematic way to investigate historical lookaheads. Reuse unchanged machine facts while detecting changes that make previous review evidence stale.

How

  1. Report every explicit decision in inventory.tsv and inventory.json, and changed contexts in report.json. Keep findings informational unless -PlookaheadFailOnFindings=true is selected.
  2. Cache validated, data-only snapshots by complete source and analyzer/generator identity. Rebind external inputs on every read. Changed grammars still run JJTree and a complete JavaCC parse/semantic check, then reuse unchanged FIRST components, call-site FOLLOW equations and decision rows. Reset invalidated components so deletions can shrink facts. CI restores and saves the cache.
  3. Keep four-column evidence as full context evidence. An explicit fifth-column token-probe scope certifies only one complete, positive, unnested token restriction. Bind full contexts to actual ancestors, their complete callee closures, continuations, lexer/options and external inputs. Opaque Java retains whole-grammar fallback; only the narrow effect-free subset can reuse independent full contexts. Token certificates cannot inherit AST or complete-cleanup claims.

Root cause

A complete one-token probe does not need an unbounded scan, but deleting its restriction can widen a called rule's FIRST set. Keep the syntactic restriction at depth one, including nested contexts.

FIRST and FOLLOW1 alone cannot validate old cleanup evidence. Longer continuations, caller prefix probes, earlier competing alternatives and preceding callees that scan beyond their call can change reachable inputs. Named inline regex definitions, lexical transitions and opaque Java also require conservative binding.

Testing

  • Ninety audit/cache/cleanup cases pass in both Gradle and Maven, including actual JavaCC generation, JDK compilation and four caller-reachability counterexamples. The unfixed tool reused stale evidence in those four cases; the current version invalidates it. Real master-to-candidate and changed-production facts/contexts agree with independent cold analysis.
  • Full local Gradle checks: 10,840 tests, 10,815 passed, 25 skipped. Full Maven verify: 10,822 tests, 10,797 passed, 25 skipped. Both have zero failures/errors; Maven excludes the eighteen HowToUseSample cases covered by Gradle. Targeted PMD 6.55 and expanded PMD 7.17 checks report no findings.
  • Eighty-six targeted SQL cases give identical results: 43 accepted and 43 rejected, with matching AST fields, deparse, EOF and successful round trips. Forty-one accepted cases have identical serialized ASTs; two EXASOL cases share the baseline DBMSType serialization limitation. Generated scan helpers fall from 1,024 to 966, with zero choice conflicts and identical TokenManager, Constants and public parser API.
  • For the real 717-production grammar, a cold snapshot load took 5.9 s and a separate JVM cache hit 1.1 s, with zero JJTree runs, JavaCC parses or fact recomputation on the hit. An analysis-only NotifyStatement action edit recomputed one FIRST component, zero FOLLOW productions and zero sites, matching forced cold analysis. Both changed and cold runs took about 4.0 s; no elapsed-time advantage is claimed for that edit. The strict HEAD report has 1,288 unchanged sites and reuses one full-context record and one token certificate.
  • Project JMH on the existing Linux dev host compares the same 54-statement corpus and identical deparse, semantic fields and serialized AST output. Four interleaved windows use three forks each, 3 x 3 s warmup and 10 x 1 s measurement per fork, one thread, 1 GiB heap and 4 MiB stack: 120 samples. Paired latency changes are -1.04% and +0.49%. Baseline/candidate score intervals are [17.217, 17.829]/[17.040, 17.643] and [17.031, 17.670]/[17.126, 17.746] ms/op. Both pairs overlap; no measurable regression was observed on this corpus.

Behavior notes

The 57 probes retain their token restrictions; state, dialect and interruption guards remain in place. EXA AT conn TABLE remains invalid after the separate DBMSDestination cleanup, but failure now occurs after consuming TABLE. Exact rejected-input diagnostic text is not promised unchanged.

The cache and analyzer remain test/build tools outside the library JAR and add no dependency. Changed grammars still pay for a full front end. Caller closures can over-invalidate full contexts, and opaque production grammars cannot claim general Java independence. The report neither rewrites grammar automatically nor proves equivalence for every SQL input or runtime configuration.

Verification of the original issue

No issue is linked. This follows the maintainer's cleanup question. The expression-dispatch work from #2827 is already upstream; this revision is one change commit on master at f6ac2571.

@fudianchn
fudianchn force-pushed the perf/lookahead-audit branch 2 times, most recently from 2700c05 to 506b5ce Compare October 19, 2026 08:34
Keep complete single-token restrictions at depth one and remove one
investigated redundant destination guard. Add a complete decision inventory,
content-addressed snapshots and incremental FIRST/FOLLOW and site reuse.

Bind full review evidence to caller reachability, transitive grammar and
opaque inputs. Keep local token certificates explicit and limited in scope.

Preserve the inherited test process environment for Windows Git fixtures and
refresh the two review bindings after the watched build input changes.

Signed-off-by: 付典 <[email protected]>
@manticore-projects

Copy link
Copy Markdown
Contributor

@fudianchn: I love this very much and I am honestly impressed by it!

But I also wonder:

  1. how much improvement does it yield for JSQLParser (e. g. how many LOOKAHEADs did it clean up and what is the JMH impact)
  2. should this not better be moved into JavaCC, so any other Grammar can also benefit from it?

@fudianchn
fudianchn force-pushed the perf/lookahead-audit branch from 506b5ce to 1c729e4 Compare October 19, 2026 09:00
@fudianchn

Copy link
Copy Markdown
Contributor Author

@manticore-projects Thanks! On the cleanup and performance question: this narrows 57 single-token lookaheads across 27 productions and removes one redundant lookahead guard.

I reran the parser JMH comparison on the same machine, using the same JDK, JVM settings and 54-statement corpus, with interleaved baseline/candidate runs. Across 12 JVM forks and 120 measurement iterations, the aggregate time for parsing the full corpus went from 17.832 to 17.539 ms/op, a 1.64% reduction.

However, the two paired comparisons were -0.05% and -3.21%, and host load was lower toward the end. The fork-bootstrap 95% interval for the aggregate change was [-2.91%, -0.40%], but that does not account for time-dependent host-load drift. The earlier run was also mixed (-1.04% and +0.49%). I would therefore not present this as a meaningful, reliably demonstrated parser speedup.

The main value is reducing unnecessary lookahead and making the grammar easier to understand and maintain. The tool provides an evidence-based way to review future LOOKAHEAD changes. From a purely performance perspective, these measurements have not established a substantial practical improvement.

@fudianchn

Copy link
Copy Markdown
Contributor Author

@manticore-projects On your second point, I have opened a draft PR for the reusable audit tooling in your JavaCC core fork: manticore-projects/javacc-8-core#1 .

The extraction and JSqlParser integration have been validated locally. The JavaCC PR documents the outstanding parent-build and artifact-publication questions. This JSqlParser PR still contains the original in-repository tooling; it has not yet switched to the extracted implementation or an unpublished JavaCC dependency. I am marking this PR ready for review with that distinction made explicit.

@fudianchn
fudianchn marked this pull request as ready for review October 19, 2026 13:17
@manticore-projects

Copy link
Copy Markdown
Contributor

This is fantastic work!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants