Bypass f-string tag round-trip for var operation operands - #6798
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
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Merging this PR will not alter performance
Comparing 
Footnotes
|
Greptile SummaryThis PR fixes a real memory leak in
Confidence Score: 5/5Safe to merge — the change is self-contained, the core correctness invariant (operand VarData reaches the merged result via _args) is unchanged, and the new tests exercise the critical paths. The implementation is narrow and well-tested. The ref-counted flag is correctly scoped to each operation invocation, frozen-dataclass dict bypass is intentional and sound (Var does not use slots, so dict is always present on all subclasses), and the VarData flow through _args + return expression is unaffected. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/vars/base.py | Adds ref-counted _format_without_tagging flag to bypass f-string tagging for var_operation operands, fixing the _global_vars memory leak and reducing var-operation construction cost ~25% |
| tests/units/vars/test_base.py | Adds two regression tests: one verifying _global_vars doesn't grow during var operations and the suppression flag cleans up correctly, another verifying body-created vars still contribute VarData |
| packages/reflex-base/news/6798.performance.md | New changelog entry describing the memory-leak fix and ~25% performance improvement for var-operation construction |
Reviews (3): Last reviewed commit: "State measured state-var operation speed..." | Re-trigger Greptile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f75b20c48
ℹ️ 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 str(self) |
There was a problem hiding this comment.
When the same Var is shared across threads, this instance-dict flag changes __format__ process-wide while an operation body is running. A concurrent f"{var}" therefore returns the raw expression without its tag, so a Var constructed from that string loses the original imports/hooks; overlapping operations can also race the non-atomic reference-count updates and leave the flag set or raise during cleanup. Use thread/task-local suppression rather than mutating the shared immutable operand.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for operand in operands: | ||
| operand_dict = operand.__dict__ | ||
| operand_dict["_format_without_tagging"] = ( # pyright: ignore[reportIndexIssue] | ||
| operand_dict.get("_format_without_tagging", 0) + 1 | ||
| ) | ||
| try: | ||
| return_var = func(*args_vars.values(), **kwargs_vars) # pyright: ignore [reportCallIssue] | ||
| finally: | ||
| for operand in operands: | ||
| operand_dict = operand.__dict__ | ||
| remaining = operand_dict["_format_without_tagging"] - 1 | ||
| if remaining: | ||
| operand_dict["_format_without_tagging"] = remaining # pyright: ignore[reportIndexIssue] | ||
| else: | ||
| del operand_dict["_format_without_tagging"] # pyright: ignore[reportIndexIssue] |
There was a problem hiding this comment.
The increment loop runs before the try block, so if it raises mid-iteration (e.g., an operand whose __dict__ is inaccessible), any operands already incremented will have _format_without_tagging stuck at a non-zero value permanently — causing them to silently bypass tagging in all future uses. This can happen today when LiteralVar.create(range_value) returns None (see LiteralVar.create line ~1802), placing None in operands; the first valid-var operand gets incremented, then None.__dict__ raises before the try, so the finally never runs. Moving the increment loop inside try (and using .get(..., 0) in the decrement to tolerate partial increments) would guarantee cleanup regardless of which step fails.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-base/src/reflex_base/vars/base.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/vars/base.py:937">
P2: Supported `NumberVar` format specs inside a `var_operation` still grow `_global_vars`: `NumberVar.__format__` returns early with a formatted nested operation before this suppression check is reached. Applying the suppression-aware raw return in the number-format branches would avoid retaining one intermediate Var per operation.</violation>
<violation number="2" location="packages/reflex-base/src/reflex_base/vars/base.py:937">
P2: Operand f-strings passed through `LiteralVar.create` inside a custom operation now become literal text instead of expressions containing the operand, because this removes the marker consumed by `LiteralStringVar.create`. Consider limiting the raw interpolation path to construction of `CustomVarOperationReturn.js_expression` or preserving a non-global marker that the literal parser can still decode.</violation>
<violation number="3" location="packages/reflex-base/src/reflex_base/vars/base.py:1961">
P2: Formatting a shared Var in another thread can silently lose its tag while any `var_operation` using that Var is running, and concurrent refcount updates can leave stale suppression state. A context-local suppression set (for example, `ContextVar` with token reset) would isolate nested operations without mutating otherwise immutable shared Vars.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| # normally and keep contributing VarData via the return expression. | ||
| for operand in operands: | ||
| operand_dict = operand.__dict__ | ||
| operand_dict["_format_without_tagging"] = ( # pyright: ignore[reportIndexIssue] |
There was a problem hiding this comment.
P2: Formatting a shared Var in another thread can silently lose its tag while any var_operation using that Var is running, and concurrent refcount updates can leave stale suppression state. A context-local suppression set (for example, ContextVar with token reset) would isolate nested operations without mutating otherwise immutable shared Vars.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/vars/base.py, line 1961:
<comment>Formatting a shared Var in another thread can silently lose its tag while any `var_operation` using that Var is running, and concurrent refcount updates can leave stale suppression state. A context-local suppression set (for example, `ContextVar` with token reset) would isolate nested operations without mutating otherwise immutable shared Vars.</comment>
<file context>
@@ -1941,10 +1948,34 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> Var[T]:
+ # normally and keep contributing VarData via the return expression.
+ for operand in operands:
+ operand_dict = operand.__dict__
+ operand_dict["_format_without_tagging"] = ( # pyright: ignore[reportIndexIssue]
+ operand_dict.get("_format_without_tagging", 0) + 1
+ )
</file context>
| # raw JS expression: their VarData flows through the operation's | ||
| # ``_args``, so the tag round-trip (and its permanent ``_global_vars`` | ||
| # entry) is pure overhead there. See ``var_operation``. | ||
| if self.__dict__.get("_format_without_tagging"): |
There was a problem hiding this comment.
P2: Supported NumberVar format specs inside a var_operation still grow _global_vars: NumberVar.__format__ returns early with a formatted nested operation before this suppression check is reached. Applying the suppression-aware raw return in the number-format branches would avoid retaining one intermediate Var per operation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/vars/base.py, line 937:
<comment>Supported `NumberVar` format specs inside a `var_operation` still grow `_global_vars`: `NumberVar.__format__` returns early with a formatted nested operation before this suppression check is reached. Applying the suppression-aware raw return in the number-format branches would avoid retaining one intermediate Var per operation.</comment>
<file context>
@@ -930,6 +930,13 @@ def __format__(self, format_spec: str) -> str:
+ # raw JS expression: their VarData flows through the operation's
+ # ``_args``, so the tag round-trip (and its permanent ``_global_vars``
+ # entry) is pure overhead there. See ``var_operation``.
+ if self.__dict__.get("_format_without_tagging"):
+ return str(self)
+
</file context>
| # raw JS expression: their VarData flows through the operation's | ||
| # ``_args``, so the tag round-trip (and its permanent ``_global_vars`` | ||
| # entry) is pure overhead there. See ``var_operation``. | ||
| if self.__dict__.get("_format_without_tagging"): |
There was a problem hiding this comment.
P2: Operand f-strings passed through LiteralVar.create inside a custom operation now become literal text instead of expressions containing the operand, because this removes the marker consumed by LiteralStringVar.create. Consider limiting the raw interpolation path to construction of CustomVarOperationReturn.js_expression or preserving a non-global marker that the literal parser can still decode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/vars/base.py, line 937:
<comment>Operand f-strings passed through `LiteralVar.create` inside a custom operation now become literal text instead of expressions containing the operand, because this removes the marker consumed by `LiteralStringVar.create`. Consider limiting the raw interpolation path to construction of `CustomVarOperationReturn.js_expression` or preserving a non-global marker that the literal parser can still decode.</comment>
<file context>
@@ -930,6 +930,13 @@ def __format__(self, format_spec: str) -> str:
+ # raw JS expression: their VarData flows through the operation's
+ # ``_args``, so the tag round-trip (and its permanent ``_global_vars``
+ # entry) is pure overhead there. See ``var_operation``.
+ if self.__dict__.get("_format_without_tagging"):
+ return str(self)
+
</file context>
4 participants
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. Every@var_operation(all arithmetic, comparisons, string/array/object ops) pays this round-trip for each operand — and for operands it is pure overhead, because theirVarDataalready reaches the operation throughCustomVarOperation._args.Fix
var_operationnow suppresses tagging on its operand vars while the body runs:finally;VarDatakeeps flowing through the return expression (covered by a dedicated test);Measured results
Memory leak (the primary win). Constructing 1000 operations with distinct operands grows
_global_varsby 1001 entries before this change, 0 after. This growth is unbounded over the life of the process and invisible to CodSpeed (no memory instrument on these benchmarks).Micro-level CPU. Local A/B against the base commit (same machine,
timeit, best of 5): constructinga + bdrops from 18.17 µs/op to 13.52 µs/op (~25% faster).CodSpeed benchmark suite: no measurable change. Head vs base shows +0.1% total impact with all 27 benchmarks "Unchanged" (e.g.
test_console_log371.7 µs → 372.2 µs,test_evaluate_page[_complicated_page]27.3 ms → 27.3 ms). The base run's flamegraph explains why: evaluate/compile time is dominated byComponent._post_init(~61% of total),LiteralVar._create_literal_var(~17%), and_isinstancechecks — var-operation construction is too thin a slice for a ~25% cut of it to move the suite. The benchmark-visible motivation for this PR is the leak fix; the CPU reduction is real but only at the micro level.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.Part of a compiler-performance series; tracked in Linear as ENG-10104.