Cache ANTLR parse trees between parses - #2566
Merged
Merged
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
shangyian
marked this pull request as ready for review
September 23, 2026 01:29
Profiling /sql/measures/v3/ and /sql/metrics/v3/ showed SQL text parsing dominating build time: `parse()` accounted for roughly 55% of a request, and up to two thirds of the calls re-parsed a string the same request had already parsed. The worst offenders were filter predicates (42 calls over 2 distinct strings on a 12-metric query), dimension link join SQL, and node query text. Memoizing `parse()` itself is not safe — the builder mutates the DJ AST it gets back, and `Function.__deepcopy__` returns `self`, so copies still alias. Caching one step lower is: `parse_rule` splits into an ANTLR parse and a visitor pass, the ANTLR tree is only ever read, and the mutable DJ AST is rebuilt on every call. Measured on the BUILD_V3 example model, with byte-identical responses across twelve metric/dimension/filter shapes: /sql/measures/v3/ 7-31% faster /sql/metrics/v3/ 5-29% faster The widest queries gain the most, since they re-parse the most.
shangyian
force-pushed
the
perf/antlr-parse-tree-cache
branch
from
September 23, 2026 01:29
baed376 to
6fdcec3
Compare
robinld
reviewed
Sep 23, 2026
|
|
||
| #: How many ANTLR parse trees to keep. Each entry retains its parser and token | ||
| #: stream, so this trades memory for parse time. | ||
| ANTLR_TREE_CACHE_SIZE = 512 |
Contributor
There was a problem hiding this comment.
feels small? I don't know how big an antlr tree is though.
Collaborator
Author
There was a problem hiding this comment.
Yeah, I ended up splitting this into two configurable caches, one for the node definitions and one for the filters (we parse a lot of filters it appears).
robinld
approved these changes
Sep 23, 2026
Filters and orderby clauses are parsed from request query params, so their variety is unbounded — every distinct literal a caller sends claimed a slot in the same cache holding node definitions. Under real traffic that churn would evict the entries worth keeping, and the hit rate would decay with nothing visibly breaking. Request SQL now uses a separate small cache. Callers opt in with `from_request=True`, which keeps the routing at the two sites that read request input rather than guessing from the SQL itself. Both caches report hits, misses, size and max size once per SQL build. Misses climbing while size sits at max size is the signal that a cache is undersized; today the example model fills 46 of 512 slots at a 98% hit rate, which says nothing about a production graph.
Replaying a day of prod v3 traffic (1,212 requests) against a prod-sized graph showed the previous split was backwards. Caching node definitions is worth nothing on this workload — it measured 4% *slower* than no cache at all, on a 52% win rate. Every bit of the gain comes from caching the SQL that arrives on the request: filters and orderby clauses. The reason is that `BuildContext.get_parsed_query` already caches parsed node ASTs within a request, so a definitions cache only helps across requests — and real traffic repeats node definitions far less than expected (755 distinct request shapes over 963 distinct metrics). Filters are the opposite: a single build parses the same predicate dozens of times. Measured against the same corpus, caches off vs on: p50 238ms -> 170ms p90 914ms -> 757ms p99 3944ms -> 3377ms mean 507ms -> 408ms (18.7%, faster on 1083/1175 requests) Dropping the definitions cache also drops the memory question it carried: each entry retained a parser and token stream for a multi-KB node query, per worker process. What remains caches `SELECT 1 WHERE <predicate>`. The size moves to `Settings.request_parse_cache_size` so it can be retuned from the counters without shipping code. The cache is built on first use because `lru_cache` binds `maxsize` at decoration time while settings resolve lazily.
The previous commit removed the node definition cache on the strength of two benchmark runs that turned out to be confounded. Those arms ran fourth and fifth in a sequence whose wall time climbed steadily run over run (468s, 597s, 615s, 672s) regardless of configuration, so the slowdown they showed was the environment, not the design. Re-measured with the three configurations interleaved, three repetitions each, so any drift spreads across all arms instead of landing on whichever ran last. Baseline reps came in at 515/503/507ms, so the environment was stable this time and the gaps below are real. config r1 r2 r3 mean vs off no caching 515 503 507 508ms - request only 482 496 501 493ms -3.0% both caches 459 457 466 460ms -9.4% Paired per request, pooling all three repetitions: both caches beat no caching on 947/1173 requests, and beat request-only caching on 855/1173. So both caches earn their place, the node definition cache more than the request one, and the change is worth about 9% on this traffic — not the 19% claimed two commits ago, which came from unreplicated runs. Both sizes now live in Settings so they can be retuned from the counters without shipping code. Measured on 400 prod v3 requests replayed against a prod-sized graph, with both caches cold at the start of every run. A long-lived worker should do better. The legacy /sql path, which carries far more traffic, is untested.
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
2 participants
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
Profiling
/sql/measures/v3/and/sql/metrics/v3/showed that sql text parsing dominated the build time with theparse()call accounting for roughly 55% of a request. Many calls toparse()would reparse strings that had already been parsed by the same request.Memoizing
parse()itself is not safe because the builder mutates the DJ AST, but caching below that at the ANTLR tree level is fine (the mutable DJ AST is rebuilt from the ANTLR tree).Measured on the
BUILD_V3examples:Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan