Bypass f-string tag round-trip for var operation operands#6747
Conversation
Interpolating a var into an f-string hashes it, permanently registers it in the module-global _global_vars dict (which never evicts - a memory leak, worst under dev hot-reload), and emits a tag that the return expression's __post_init__ regex-decodes back out. For operands of a @var_operation this round-trip is pure overhead (~30-40% of op construction): the operation merges operand VarData directly from _args. var_operation now suppresses tagging on its operands (ref-counted, so nested operations on the same var can't clear an outer suppression early) while the body runs. Vars created inside the body still tag normally, so their VarData keeps flowing through the return expression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy8uHH11KircGa2MbTrR8g
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy8uHH11KircGa2MbTrR8g
Greptile SummaryThis PR removes the f-string tag round-trip for variable-operation operands. The main changes are:
Confidence Score: 5/5No additional blocking issue qualifies for this review round.
Important Files Changed
Reviews (2): Last reviewed commit: "Apply suggestion from @greptile-apps[bot..." | Re-trigger Greptile |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b4893e8bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.__dict__.get("_format_without_tagging"): | ||
| return self._js_expr |
There was a problem hiding this comment.
Preserve tags when operands feed string Var builders
Because this flag is checked by every Var.__format__ call while a custom var_operation body runs, operands no longer produce the tagged f-string form that LiteralStringVar.create/Var.create depend on to turn f"prefix {value}" into a JS string concat. In that scenario the operand now formats as raw state.name, so the string builder compiles a literal like "prefix state.name" instead of a value depending on state.name; the _args merge only preserves VarData, not the string-concat semantics. Consider limiting the bypass to the var_operation_return JS-expression path rather than the whole body.
Useful? React with 👍 / 👎.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
masenf
left a comment
There was a problem hiding this comment.
vars are supposed to be immutable.
yes, we already do some tricks with the cached var, but that is idempotent. this patch sneaks statefulness back into the var for tracking this counter... but it does seem to be a nice optimization.
i'm trying to think of a way to practically repro the edge case that was mentioned in a review comment...
All Submissions:
Type of change
Changes To Core Features:
What this does
Interpolating a
Varinto an f-string (Var.__format__) hashes the var, permanently registers it in the module-global_global_varsdict (which never evicts — a real memory leak, worst under dev hot-reload since entries and theirGLOBAL_CACHEvalues survive GC), and emits a<reflex-var>tag that the constructed expression's__post_init__then regex-decodes back out. Profiling showed this round-trip is ~30–40% of var-operation construction — and every@var_operation(all arithmetic, comparisons, string/array/object ops) pays it for each operand.Fix
Operand
VarDataalready reaches the operation throughCustomVarOperation._args, so the tag round-trip is pure overhead for operands.var_operationnow suppresses tagging on its operand vars while the body runs:finally;VarDatakeeps flowing through the return expression (covered by a dedicated test);Net effect: constructing
a + bno longer hashes/registers/regex-decodes its operands, and internal ops no longer grow_global_vars.Verification
_global_varsdoes not grow when constructing an operation while operandVarDatastill reaches the merged result; the suppression flag does not persist after the op; body-created vars keep contributingVarData; formatting outside an operation still registers/tags as before.test_console_log,test_evaluate_page*, and alltest_compile_*benchmarks.Part of a compiler-performance series; tracked in Linear as ENG-10104.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Jy8uHH11KircGa2MbTrR8g
Generated by Claude Code