Add holistic table event processing benchmark - #7061
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
 The benchmark coverage improvement is useful, but the explicit repository requirement concerning hardcoded identifier keys remains unresolved and must be satisfied before merging. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| "name": f"order {i}", | ||
| "customer": f"customer {i % 50}", | ||
| "amount": i * 1.5, |
There was a problem hiding this comment.
These assertions use "status", "sort_reverse", "filtered_orders", and "total_amount" directly as dictionary keys. This violates the repository directive that string literals used as identifiers or keys must be extracted into named constants, and it can let the assertions drift if the state fields are renamed. This repository requirement must be satisfied before merging.
Rule Used: String literals that are used as identifiers or ke... (source)
Learned From
reflex-dev/flexgen#2170
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Merging this PR will improve performance by 3.03%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_var_access[mutable_dict] |
47 ms | 45.6 ms | +3.03% |
| 🆕 | test_process_event[counter] |
N/A | 11.4 ms | N/A |
| 🆕 | test_process_event[table] |
N/A | 694.1 ms | N/A |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing FarhanAliRaza:benchmark/holistic-table-events (8dedc2d) with main (5d9724e)
Footnotes
-
9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| await p.enqueue(token, event) for _ in range(num_events) | ||
| await p.enqueue("benchmark-token", event) for event in events | ||
| ]): | ||
| pass |
There was a problem hiding this comment.
The refactor removes test_table_event_deltas, while run_events now checks only how many updates were emitted. The serialized table updates are discarded without validating their contents, so a regression that omits or corrupts the filtered rows, sort direction, or computed total could still pass and appear as a performance improvement. Please retain a separate correctness test for the serialized table deltas.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
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="tests/benchmarks/test_event_processing.py">
<violation number="1" location="tests/benchmarks/test_event_processing.py:39">
P2: The counter workload's state grows across benchmark samples: each batch runs three `increment` events and never restores state, so `counter` rises by 3 per invocation. Because `elements`, `nested_elements`, `show_odd`, and `show_even` are computed vars depending on `counter`, every subsequent sample recomputes and serializes strictly larger structures, so the measured cost is not constant. This contradicts the PR's stated goal of repeatable samples (the table workload restores via an even number of sort-reversal toggles; the counter workload has no restore). CodSpeed aggregates multiple invocations, so results mix an increasing workload and are not reproducible. Reset `counter` (or re-run from a fixed state) at the end of each sample so every benchmark invocation measures the same work.</violation>
<violation number="2" location="tests/benchmarks/test_event_processing.py:60">
P2: Keep a separate correctness assertion for the serialized table updates instead of discarding the encoded result here. Counting emitted deltas will not detect missing or corrupted filtered rows, sort direction, or computed totals.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if table | ||
| else BenchmarkState.event_handlers["increment"] | ||
| ) | ||
| payloads = ( |
There was a problem hiding this comment.
P2: The counter workload's state grows across benchmark samples: each batch runs three increment events and never restores state, so counter rises by 3 per invocation. Because elements, nested_elements, show_odd, and show_even are computed vars depending on counter, every subsequent sample recomputes and serializes strictly larger structures, so the measured cost is not constant. This contradicts the PR's stated goal of repeatable samples (the table workload restores via an even number of sort-reversal toggles; the counter workload has no restore). CodSpeed aggregates multiple invocations, so results mix an increasing workload and are not reproducible. Reset counter (or re-run from a fixed state) at the end of each sample so every benchmark invocation measures the same work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/benchmarks/test_event_processing.py, line 39:
<comment>The counter workload's state grows across benchmark samples: each batch runs three `increment` events and never restores state, so `counter` rises by 3 per invocation. Because `elements`, `nested_elements`, `show_odd`, and `show_even` are computed vars depending on `counter`, every subsequent sample recomputes and serializes strictly larger structures, so the measured cost is not constant. This contradicts the PR's stated goal of repeatable samples (the table workload restores via an even number of sort-reversal toggles; the counter workload has no restore). CodSpeed aggregates multiple invocations, so results mix an increasing workload and are not reproducible. Reset `counter` (or re-run from a fixed state) at the end of each sample so every benchmark invocation measures the same work.</comment>
<file context>
@@ -28,34 +19,45 @@
+ if table
+ else BenchmarkState.event_handlers["increment"]
+ )
+ payloads = (
+ [{"status": status} for status in ("open", "", "paid") * 2]
+ if table
</file context>
| nonlocal emitted | ||
| emitted += 1 | ||
| if table: | ||
| json_dumps(StateUpdate(delta=delta), separators=(",", ":")) |
There was a problem hiding this comment.
P2: Keep a separate correctness assertion for the serialized table updates instead of discarding the encoded result here. Counting emitted deltas will not detect missing or corrupted filtered rows, sort direction, or computed totals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/benchmarks/test_event_processing.py, line 60:
<comment>Keep a separate correctness assertion for the serialized table updates instead of discarding the encoded result here. Counting emitted deltas will not detect missing or corrupted filtered rows, sort direction, or computed totals.</comment>
<file context>
@@ -28,34 +19,45 @@
emitted += 1
- on_delta(delta)
+ if table:
+ json_dumps(StateUpdate(delta=delta), separators=(",", ":"))
async def emit_event_impl(token: str, *events: Event) -> None:
</file context>
1 participant
Add an order-management workload to measure backend event performance beyond counter increments. Six events filter and sort 1,000 dataclass rows, recompute totals, generate deltas, and serialize StateUpdate envelopes through Reflex's JSON encoder.
The benchmark warms state before timing and returns to the same filter and sort direction after each sample. A separate correctness test verifies serialized rows and totals across repeated batches. The README documents how to run it and the measurement boundary: processor lifecycle is included; hydration, network transport, database access, and browser rendering are excluded.
Related to #7056, which includes the original benchmark alongside runtime optimizations. This PR isolates the benchmark and adds repeatable samples, correctness assertions, and run documentation. Encoding is measured through
json_dumps; changes confined to Socket.IO's encoder configuration require updating the benchmark's encoder too.Validation
uv run pyright reflex tests: passed.git diff --check: passed.Repository-wide Ruff checks report unrelated existing issues under
.states.bak/andignore/. The full unit suite was not run; this change only adds benchmark coverage and documentation.No package source or public behavior changes; no changelog fragment included.