perf(sql): restore packrat memoisation - 500 grammar productions from def to lazy val - #339
Merged
Merged
Conversation
…lazy val PackratParsers memoises on (parser INSTANCE, position) and the library's own scaladoc requires a production to be a `lazy val`. A `def` builds a fresh instance on every reference, so each reference gets its own memo entry and identical work is re-done at the same input position - memoisation silently off for the whole grammar. Declaration form only: every nullary `PackratParser` production becomes a `lazy val`. The single parameterised production (`relationCriteria`) and the two abstract declarations stay `def`; no alternation is reordered and no production body changes. Per-statement parse latency (median of 1,000 parses, 5 JVM runs) drops from 10.16 ms to 3.17 ms over the ten-shape probe set (-68.9 %); ParserSpec wall time 1.774 s -> 1.456 s (-17.9 %, non-overlapping ranges), both measured against an origin/main control re-run in the same session. Zero behaviour change, demonstrated rather than assumed: 15,601 inputs replayed through parsers built from both trees produced byte-identical verdict files (same SHA-256), 0 differing verdicts, 0 differing renders, 0 throws on either side. The probe was shown to fire on a one-alternative canary mutation. Adds a non-asserting `ParseCostProbe`, the `GrammarDiffProbe` that produced the differential result, and `ParserConcurrencySpec` (8 threads against a COLD `object Parser` in a child-first classloader, plus the ambient one) - the one hazard the shared-instance form introduces. Closed Issue #338
fupelaqu
marked this pull request as ready for review
September 15, 2026 06:58
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
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.
Closes #338
PackratParsersmemoises on (parser INSTANCE, input position) and the library's own scaladoc requires each nullary production to be alazy val. Adefbuilds a fresh instance on every reference, so each reference site gets its own memo entry, no alternative can hit another's, and identical work is re-done at the same position — memoisation silently off for the whole grammar.This PR changes declaration form only. 500 nullary
PackratParserproductions becomelazy val. No alternation is reordered, no production body is edited. The visible line count is larger than 500 becauselazy valis five characters longer thandef, so scalafmt re-wrapped many bodies;git diff -woversql/src/mainshows nothing but the declarations and those re-wraps.What was converted, what was left alone
PackratParserproductions →lazy valPackratParserdeclarations left asdefderivedTableBodyInnerandderivedTableare declared ontrait Parserand implemented elsewhere. Alazy valmay implement a deferreddef, not the reverse, so the declarations staydefand both implementations are nowlazy valdefrelationCriteria(relation: String)— each call is a different parser, so it cannot be memoised on identityParser[...]-typed productions left asdefContribution by file group
Cumulative, measured after each batch with the committed
ParseCostProbe(3 JVM runs per intermediate row, 5 for the endpoints). Marginal contributions are order-dependent — a later group can only claim what earlier groups have not already taken — so the order is stated, not implied.origin/maincontrolparser/WhereParser.scalaparser/Parser.scalaparser/type/package.scalaparser/function/**(time, aggregate, math, geo, string, cond, convert)parser/time,parser/http,parser/operator/mathTwo rows worth reading honestly:
WhereParserproductions (any_identifier,criteria) were alreadylazy valonmain. The other 47 were not where the cost was.Parser.scalaholds the identifier, value and literal productions that every other production funnels through.Before / after, against a control re-measured in the same session
Absolute parse timings drift between sessions, so the
origin/maincontrol was re-run on the same quiesced machine immediately before the branch numbers, using the same probe binary and the same JVM settings — 5 JVM runs per side, each reporting the median of 1,000 timed parses after 300 discarded warm-up iterations.SELECTSELECTlist +LIMITGROUP BY+HAVINGJOINCREATE TABLEEvery per-statement range is non-overlapping between the two sides.
Secondary instrument,
ParserSpecwall time (ScalaTest "Run completed in", 5 runs per side, 315 tests green on both):origin/main)−17.9 %, ranges non-overlapping. The suite moves less than the probe because most of its wall time is fixture construction and ScalaTest overhead rather than parsing — which is exactly why per-statement latency is the number that matters to a user: every query they send pays it.
Zero behaviour change — demonstrated, not asserted
A branch-only suite cannot establish "no narrowing": the inputs a grammar change breaks are by construction the ones nobody wrote a test for. So the claim is backed by a differential replay, and the harness ships with the PR (
GrammarDiffProbe).examples[].sqlandsyntax[]line of the help corpus, every SQL-shaped string literal in thesqltest sources, every whitespace-boundary prefix of all of those, and targeted mutations (stray(/), danglingAND/OR, trailing comma, unterminated quote, misspeltWHERE/FROM/SELECT).origin/mainand from this branch, recording per input: verdict, theParserErrormessage on rejection, and on success both the AST render (Statement.sql) and itstoString.identifierWithArithmeticExpression) fromany_identifieron the control tree produced 14 differing rows. A zero from an instrument never demonstrated to be non-zero is worth nothing.New concurrency guard
Sharing one parser instance is the one hazard this change introduces, and no existing test covered it.
ParserConcurrencySpecadds three:object Parser, loaded in a child-firstURLClassLoaderwith anullparent so every production is uninitialised when the threads hit it. This leg carries its own integrity gate — it asserts the class came back from that loader and fails loudly if delegation ever made it a second copy of the warm leg (falsified: pointing the loader's parent at the ambient classloader turns the leg red with that message).Parser, compared to the single-threaded answers.Every leg is bounded by a timeout, so an initialisation deadlock fails the suite instead of hanging CI. The structural argument it checks is recorded in the file: a
lazy valon an object initialises under that object's single monitor (no lock-ordering inversion possible between productions), andparser2packrattakes its argument by name, so a production's body is evaluated on first application, not on initialisation — which is what keeps the mutually recursive productions safe.New parse-cost probe
ParseCostProbeis a runnable, non-assertingmainundersql/src/test:It times ten statement shapes (bare SELECT, SELECT list, WHERE-heavy, GROUP BY + HAVING, window, JOIN, derived table, WHERE subquery, DDL, and a deliberate rejection), printing runs / median / min / p95 / max. It discards a global warm-up plus 300 further iterations per statement before timing, and it is locale-independent so the table stays machine-readable.
There is no timing assertion anywhere in the suite, by design. A wall-clock ceiling in CI is a flake liability — the previous
ParseCostProbeSpechad to have its 1 ms ceiling relaxed to 10 ms (#269/#270) after it reddened two PRs whose diffs touched no parse path. This probe prints; it never fails.Candidates that bought nothing (measured, then reverted)
The 19 nullary
Parser[...]-typed productions (arithmeticExpressionLevel1/2,identifierWithValue,case_condition,ident, …). Converting them tolazy valavoids re-allocating the combinator tree per reference but enables no memoisation, since the packrat cache only ever sees aPackratParser. Measured at −0.9 % against the converted branch, with fully overlapping ranges — noise. Reverted: it widens the blast radius into the hottest shared productions for a number that cannot be measured. Recorded here rather than carried.Two consequences worth stating
Parser$gains 45 new public methods, and loses none. They are all name-mangled trait-private accessors (app$softnetwork$…$MathParser$$abs()and friends): a trait-privatedefcompiles to a private method, while a trait-privatelazy valneeds a mangled but public accessor in the mixing class. Verified by diffingjavapoutput ofParser$built from both trees — 45 added, 0 removed, 0 changed, and every added entry carries$$. So this is additive and no source-level API moves. Anything that mixes in the parser traits (nothing in this repo or the sibling repos does — they use theParserobject) would need a rebuild.HelpCorpusSpec's reflection scan enumerates productions returning a sealed trait, andWhereParser's trait-privatesubqueryBodybecame visible togetMethodsfor the reason in (1). It is added to the expected set with that explanation; it names no new statement leaf (its body isderivedTableBodyInnerin parentheses) so the AST package walk's coverage is unchanged.DialectCensus's source anchors, which quote declaration heads verbatim, were retargeted the same mechanical way (def x:→lazy val x:) — that file's "anchor resolves exactly once" assertion is what caught them.Verification
Run on the final tree, 2.13 only, exactly as CI does (
+ compileis main sources on both legs; CI never compiles or publishes 2.12 test sources):sql/testcore/testmacrosTests/testsoftclient4es-sql-bridge/test(template)es6bridge/testsoftclient4es9-sql-bridge/test(under the JDK-17 wrapper)+ sql/compile,+ core/compileheaderCheck scalafmtSbtCheck scalafmtCheck test:scalafmtCheckElasticsearch integration tests were not run, and that is deliberate. This PR changes no emitted query, no client code and no module outside
sql/src/main/scala/.../parserplus three test files; the bridge suites above are the layer that pins the generated Elasticsearch JSON, and they are green in both copies. The differential replay is a strictly stronger statement about the grammar than any integration run could make. CI's own matrix covers the rest.sql/Test/headerCheckstill reports 31 files without licence headers — all pre-existing, none of them added here (the three new test sources carry headers). CI runs the Compile-scopedheaderCheck, which is green.Follow-ups spotted and NOT taken
Parser.applydoes an unconditionalConsole.err.println(msg)on every rejection, from thesqlmodule — noisy stderr inside an embedded host. Pre-existing, unrelated to this diff, untouched here.Parser.applydiscards theNoSuccessreader, so the offset that would let a client show a caret is thrown away one frame before any caller could use it. Pre-existing.Parser[...]-typed productions above remaindef. If a future change makes any of them hot, the measurement to redo is in this PR body.