Skip to content

perf(gfql): build the row-expression transformer class once, not per parse (compile 2.5-2.8x) - #1837

Open
lmeyerov wants to merge 1 commit into
fix/gfql-clear-caches-real-targetsfrom
perf/gfql-transformer-class-once
Open

perf(gfql): build the row-expression transformer class once, not per parse (compile 2.5-2.8x)#1837
lmeyerov wants to merge 1 commit into
fix/gfql-clear-caches-real-targetsfrom
perf/gfql-transformer-class-once

Conversation

@lmeyerov

Copy link
Copy Markdown
Contributor

Stacked on #1836. That PR makes gfql_clear_caches() actually clear the Cypher AST memo, which is what allows a compile measurement to prove it was cold. Every number below comes from a run that asserts cache_info().currsize == 0 before each timed call and == 1 after, and aborts otherwise. Review #1836 first.

What was slow, and it was not the parser

_build_transformer() defined two @dataclass(frozen=True) helpers (_FunctionArgs, _CaseArm) and the Lark Transformer subclass _AstBuilder inside its own body. So all three types were re-created on every call — and @dataclass generates __init__/__eq__/__hash__ as Python source and execs it, meaning each rebuild ran the compiler.

_parse_expr_cached calls 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:

0.489  compile_cypher_query                          (75% of the whole compile)
0.399    _parse_row_expr          lowering.py:707     1550 calls = 50 per compile
0.328      parse_expr             expr_parser.py:775  1674 calls
0.326        _parse_expr_cached   expr_parser.py:789   434 misses (1240 hits)
0.261          _build_transformer expr_parser.py:336   434 calls  <-- 40% OF ALL COMPILE TIME
0.241            dataclasses._process_class            868 calls
0.120              builtins.exec                      868 calls (tottime)
0.171  parse_cypher                                  (26%), of which the LALR parse is 0.151

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: _AstBuilder is only incidentally stateless today (its __init__ just forwards visit_tokens=True and 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 Transformer comes from the lazy _lark_imports() — a module-level class statement 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.

q lower BEFORE lower AFTER speedup COLD compile BEFORE COLD compile AFTER speedup
q1 3.328 / 3.322 0.885 / 0.903 3.7× 4.142 / 4.132 1.640 / 1.676 2.5×
q5 6.022 / 5.981 1.423 / 1.444 4.2× 7.175 / 7.115 2.506 / 2.555 2.8×
q7 7.446 / 7.377 1.835 / 1.858 4.0× 8.816 / 8.729 3.154 / 3.206 2.8× (−5.6 ms)
q8 0.967 / 0.974 0.325 / 0.330 3.0× 1.651 / 1.658 0.959 / 0.986 1.7×

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_cached without 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.sh clean (including the type-hygiene guard — the first draft added a net cast; typing the factory as Callable[[], _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.
  • dgx, same container: the two new/updated test files 17 passed; graphistry/tests/compute/gfql/ 8175 passed, 53 skipped, 19 xfailed.
  • New test_expr_transformer_class_built_once.py pins 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 and count(DISTINCT ...) specifically, since those exercise the two hoisted dataclasses.

Not in this PR

cypher/parser.py has 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

…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
@lmeyerov

Copy link
Copy Markdown
Contributor Author

Full 10-query run: the q5 warm anomaly is resolved, and every query improves on every metric

The 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. lower = compile_cypher_query; COLD = parse + lower with the memo emptied per iteration; warm = memo populated, which is what the published cold board cell actually pays.

q lower B lower A × COLD B COLD A × warm B warm A
q1 3.385 0.893 3.79 4.221 1.656 2.55 0.657 0.575
q2a 3.371 0.887 3.80 4.186 1.647 2.54 0.656 0.594
q2b 1.667 0.395 4.22 2.349 1.040 2.26 0.363 0.310
q3 3.317 0.716 4.63 4.633 1.915 2.42 0.650 0.543
q4 2.729 0.616 4.43 4.172 1.844 2.26 0.554 0.476
q5 6.117 1.430 4.28 7.276 2.669 2.73 1.008 0.898
q6 6.549 1.464 4.47 7.826 2.677 2.92 1.343 1.153
q7 7.498 1.813 4.14 8.871 3.123 2.84 1.507 1.137
q8 0.817 0.321 2.55 1.549 0.961 1.61 0.308 0.276
q9 0.877 0.366 2.40 1.978 1.483 1.33 0.341 0.307

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-bench

pyg-bench #130, merged dde9be5results/gfql-compile-cost-20260729/ with the runner, all six raw console transcripts, and aggregate.json. Also records the Kuzu framing, which the numbers change: against Kuzu's ~0.3–0.5 ms compile, the warm arm is now ~1–3×, not the withdrawn "roughly 20×" that was quoting a cold arm against a competitor that has no cold arm.

What this does not claim

It should not move the published cold board cells much, because those already pay only the warm cost. The win lands on cold-process, first-call latency, and any workload that does not repeat queries. Re-measuring the graphbench-compile-arms boards is owed regardless, since that lane's cold-process arm turned out to be mislabeled.

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.

1 participant