diff --git a/graphistry/compute/gfql/expr_parser.py b/graphistry/compute/gfql/expr_parser.py index 08d3aadc2f..46d40fea2a 100644 --- a/graphistry/compute/gfql/expr_parser.py +++ b/graphistry/compute/gfql/expr_parser.py @@ -341,7 +341,32 @@ def _parse_number_token(token: str) -> Union[int, float]: return int(token) -def _build_transformer() -> _TransformerLike: +@lru_cache(maxsize=1) +def _ast_builder_class() -> Callable[[], _TransformerLike]: + """Define the row-expression transformer class ONCE per process, and return the class. + + This used to be the body of ``_build_transformer``, which meant the three types below -- + two ``@dataclass(frozen=True)`` helpers and the Transformer itself -- were re-created on + every call. ``@dataclass`` generates ``__init__``/``__eq__``/``__hash__`` as source and + ``exec``s it, so each rebuild ran the compiler. + + That was the single largest cost in GFQL query compilation. Profiling q7 of the matched + graph-benchmark (31 cold compiles, 0.649 s total) attributed **0.261 s -- 40% of all + compile time -- to this function**: 434 calls (one per ``_parse_expr_cached`` miss) + producing 868 dataclass creations, 0.120 s of which was ``builtins.exec`` alone. By + comparison the whole LALR(1) parse was 0.151 s. Lowering, not parsing, dominates GFQL's + compile, and this was most of lowering. + + Returning the CLASS rather than an instance is deliberate: ``_build_transformer`` still + constructs a fresh transformer per call, so no state is shared between parses and the + change is a pure construction-cost win with no re-entrancy or thread-safety question to + argue about. Only the type creation is hoisted, and a type is a function of the code. + + Cached with ``maxsize=1`` rather than defined at module scope because ``Transformer`` comes + from ``_lark_imports()``, which is lazy: a module-level class statement would make importing + this module require lark, breaking the minimal installs that only touch the non-expression + surfaces. + """ _, Transformer, _ = _lark_imports() def _is_token(value: Any) -> bool: @@ -717,7 +742,25 @@ def is_null(self, items: Sequence[Any]) -> IsNullOp: def is_not_null(self, items: Sequence[Any]) -> IsNullOp: return IsNullOp(value=cast(ExprNode, _strip_tokens(items)[0]), negated=True) - return cast(_TransformerLike, _AstBuilder()) + return _AstBuilder + + +register_process_singleton( + _ast_builder_class, + "returns the row-expression Transformer CLASS, a function of the code; cached because " + "dataclass re-execs generated source on every rebuild (40 percent of GFQL compile time); " + "instances are still created per parse, so nothing stateful is shared", +) + + +def _build_transformer() -> _TransformerLike: + """A fresh transformer instance, over a class built once per process. + + Instantiation is cheap -- ``_AstBuilder.__init__`` only forwards ``visit_tokens=True`` to + Lark and assigns no attributes. Building the class is what was expensive; see + ``_ast_builder_class``. + """ + return _ast_builder_class()() def _rebuild_expr_node( diff --git a/graphistry/tests/compute/gfql/test_expr_transformer_class_built_once.py b/graphistry/tests/compute/gfql/test_expr_transformer_class_built_once.py new file mode 100644 index 0000000000..a0231f5fb5 --- /dev/null +++ b/graphistry/tests/compute/gfql/test_expr_transformer_class_built_once.py @@ -0,0 +1,88 @@ +"""The row-expression transformer CLASS is built once per process; instances are not shared. + +Why this is worth a test rather than just a commit message: the expensive thing was never +parsing. Profiling q7 of the matched graph benchmark (31 cold compiles, 0.649 s total) put +**0.261 s — 40% of all compile time — in ``_build_transformer``**, which was re-creating two +``@dataclass(frozen=True)`` helpers and a Lark ``Transformer`` subclass on every +``_parse_expr_cached`` miss: 434 calls producing 868 dataclass creations, 0.120 s of it in +``builtins.exec`` because ``@dataclass`` generates ``__init__``/``__eq__`` as source and execs +it. The whole LALR(1) parse, by comparison, was 0.151 s. + +So the invariant has two halves and both matter. Build the class **once** (or the cost returns), +and keep instantiating **per parse** (or a future stateful transformer silently starts sharing +state between unrelated queries). A test that only checked the first half would bless a +regression into the second. +""" + +from __future__ import annotations + +import pytest + + +def test_the_transformer_class_is_created_once_per_process() -> None: + """The whole point: identical class object, so no dataclass rebuild.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import _ast_builder_class + + first = _ast_builder_class() + assert _ast_builder_class() is first, ( + "the transformer class is being rebuilt; @dataclass will re-exec generated source on " + "every row-expression parse, which was 40% of GFQL compile time" + ) + + +def test_each_parse_still_gets_its_own_transformer_instance() -> None: + """The other half. Sharing one instance would be a bigger win and a worse idea: Lark + transformers are only incidentally stateless here, and a shared instance would make that + an unwritten requirement of every future method added to the class.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import _ast_builder_class, _build_transformer + + a = _build_transformer() + b = _build_transformer() + assert a is not b, "transformer instances are being shared between parses" + assert type(a) is type(b) is _ast_builder_class() + + +def test_the_transformer_class_survives_a_cache_clear() -> None: + """``gfql_clear_caches()`` must NOT drop it — it is a function of the code, not of any + query, exactly like the Lark parser objects. Clearing it would put class construction back + inside every 'cold' measurement, which is a cost no caller can pay twice.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import _ast_builder_class + from graphistry.compute.gfql_unified import gfql_clear_caches + + before = _ast_builder_class() + gfql_clear_caches() + assert _ast_builder_class() is before + + +@pytest.mark.parametrize( + "expr", + [ + "age > 30", + "toLower(name) = 'bob'", + "a.x IS NULL", + "a.x IS NOT NULL", + "CASE WHEN age > 30 THEN 1 ELSE 0 END", + "count(DISTINCT a.id)", + "(age > 30) AND (score < 5.5)", + "a.x IN [1, 2, 3]", + ], +) +def test_parsing_is_unchanged_by_the_hoist(expr: str) -> None: + """Parity. Hoisting a class definition must not move a single AST node. + + Also exercises the two hoisted dataclasses specifically: ``_CaseArm`` via CASE/WHEN and + ``_FunctionArgs`` via a DISTINCT aggregate — they are closed over by the transformer, so a + botched hoist would surface here rather than in a generic expression. + """ + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import parse_expr + from graphistry.compute.gfql_unified import gfql_clear_caches + + first = repr(parse_expr(expr)) + gfql_clear_caches() + assert repr(parse_expr(expr)) == first, ( + f"{expr!r} parses differently across a cache clear" + )