perf(gfql): build the row-expression transformer class once, not per parse (compile 2.5-2.8x) - #1837
Conversation
…parse _build_transformer() defined two @DataClass(frozen=True) helpers and the Lark Transformer subclass INSIDE its own body, so all three types were re-created on every _parse_expr_cached miss. @DataClass generates __init__/__eq__ as source and execs it, so each rebuild ran the compiler. Profiling q7 of the matched graph benchmark (31 cold compiles, 0.649s total) attributed 0.261s -- 40% of ALL compile time -- to this one function: 434 calls producing 868 dataclass creations, 0.120s of it in builtins.exec. The whole LALR(1) parse was 0.151s by comparison. Lowering, not parsing, dominates GFQL compile, and this was most of lowering. The class is now built once per process behind lru_cache(maxsize=1); instances are still created per parse, so nothing stateful is shared and there is no re-entrancy question to argue. Only type creation is hoisted, and a type is a function of the code, not of any query -- so it is classified EXEMPT in the clear-caches enumeration, alongside the Lark parser objects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx
Full 10-query run: the q5 warm anomaly is resolved, and every query improves on every metricThe PR body reports a 4-query subset and flags q5's warm arm as an unexplained apparent regression (0.999 → 1.062 ms, non-overlapping slot ranges) on a path this patch cannot touch. The full run resolves it: q5 warm is 1.008 → 0.898 ms, faster, in line with every other query. The subset was misleading; calling it noise at the time would have been right by luck rather than by evidence. All 10 queries, 31 runs each, 3 interleaved slots, one container per slot, load 0.64–1.44.
Lowering 2.40–4.63×. Cold compile 1.33–2.92×. Warm 1.07–1.33×, every query faster. No arm overlaps; slot spread under 2% on both sides. Locked in pyg-benchpyg-bench #130, merged What this does not claimIt should not move the published |
What was slow, and it was not the parser
_build_transformer()defined two@dataclass(frozen=True)helpers (_FunctionArgs,_CaseArm) and the LarkTransformersubclass_AstBuilderinside its own body. So all three types were re-created on every call — and@dataclassgenerates__init__/__eq__/__hash__as Python source andexecs it, meaning each rebuild ran the compiler._parse_expr_cachedcalls it on every miss, and lowering a single query parses dozens of row expressions.cProfile of q7 from the matched graph benchmark, 31 cold compiles, 0.649 s total:
868 dataclass creations per 31 compiles — 28 per compiled query — for 40% of compile time. The entire LALR(1) parse was 0.151 s against 0.261 s spent re-defining classes. Lowering, not parsing, dominates GFQL's compile cost, and this was most of lowering.
The change
_ast_builder_class()is@lru_cache(maxsize=1)and returns the class._build_transformer()still instantiates a fresh transformer per call.Returning the class rather than a shared instance is deliberate. Sharing one instance would be marginally faster and considerably worse:
_AstBuilderis only incidentally stateless today (its__init__just forwardsvisit_tokens=Trueand assigns nothing), and sharing would make statelessness an unwritten requirement of every method anyone adds later, plus raise a thread-safety question. Instantiation is cheap; building the class was the cost. Only type creation is hoisted, and a type is a function of the code, not of any query.It is cached rather than defined at module scope because
Transformercomes from the lazy_lark_imports()— a module-levelclassstatement would make importing this module require lark, breaking the minimal installs that never touch the expression surface.Being a new process-lifetime cache, it is classified in #1836's enumeration as EXEMPT, next to the Lark parser objects, with the reason recorded. #1836's test failed until I did that, which is the test doing its job.
Measured
dgx-spark,
graphistry/test-rapids-official:26.02-gfql-polars,--gpus all --network none, 31 runs per query, one container per slot, BEFORE/AFTER interleaved across 3 slots, load 1.23→1.44. BEFORE = #1836 (35cf7dac), the only thing varied.Whole-compile range 1.65–8.82 ms → 0.96–3.21 ms. Slot-to-slot spread is under 2% on both sides, and the arms never overlap.
One thing I cannot explain, reported rather than dropped. The warm arm (memo hit) improves on q1 (0.654→0.571), q7 (1.488→1.153) and q8 (0.303→0.276), but q5 goes 0.999 → 1.062, and those slot ranges do not overlap. A warm hit returns from
_parse_expr_cachedwithout calling the factory at all, so this change should not touch that path. It is ~6% on one query, I have 2 BEFORE slots against 3 AFTER, and it may be cross-container variance — but I am not calling it noise without having shown that.Correctness
./bin/lint.shclean (including the type-hygiene guard — the first draft added a netcast; typing the factory asCallable[[], _TransformerLike]removes the need for any cast, since a class is a callable returning an instance).MYPY_CMD="uvx mypy==2.3.0" ./bin/mypy.sh→ no issues in 327 source files.graphistry/tests/compute/gfql/8175 passed, 53 skipped, 19 xfailed.test_expr_transformer_class_built_once.pypins both halves of the invariant — the class is identical across calls (or the cost returns) and instances are distinct (or a future stateful transformer silently shares state between unrelated queries). A test asserting only the first would bless a regression into the second. It also pins that a cache clear does not rebuild the class, and checks parse parity across a clear for CASE/WHEN andcount(DISTINCT ...)specifically, since those exercise the two hoisted dataclasses.Not in this PR
cypher/parser.pyhas its own_build_transformer(source)with the same class-defined-inside shape. It takes the query text, so it cannot be a plain singleton, and it is called once per query parse rather than once per row expression — a smaller win. Worth a follow-up; not bundled here.🤖 Generated with Claude Code
https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx