Skip to content

Stabilize flaky CodSpeed benchmarks and speed up container type checks - #7878

Merged
FarhanAliRaza merged 5 commits into
mainfrom
farhan/upbeat-thompson-ftzgzb
Nov 13, 2026
Merged

FarhanAliRaza merged 5 commits into
mainfrom
farhan/upbeat-thompson-ftzgzb

Conversation

@FarhanAliRaza

@FarhanAliRaza FarhanAliRaza commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • Bug fix (non-breaking change which fixes an issue): CodSpeed benchmarks that flip on unrelated PRs
  • Performance improvement in reflex-base: container type validation

Describe your changes

CodSpeed has been flagging the same handful of benchmarks on PRs that don't touch the code they measure. A tally of the CodSpeed reports on the 50 most recently updated PRs showed them landing on the same two values each time:

Benchmark Flagged on Values
route_matching[1000] 9 PRs −5.5 to −5.9% and +5.4 to +5.8%
state_collection_assignment[1000] 4 PRs −11 to −12% and +14%
state_collection_assignment[100000] 3 PRs ±5%
process_event_burst_independent_tokens[1] 4 PRs −4.4 to −4.5% (one PR only changes release tests)

1. test_route_matching measured three cache hits. get_router memoizes each path with lru_cache, and CodSpeed calls a benchmark once before measuring it, so the measured call was three cache lookups. That's about 8k instructions plus ~360 cold cache misses, which dominated the estimate, so table size didn't register. It now calls the wrapped scan for 100 lookups covering the index, a dynamic route, the catch-all, static routes spread over the table and paths no route matches. Cost now grows with the table: 1.3M, 11M and 111M instructions under callgrind for 10, 100 and 1000 routes. The cached_rxconfig fixture is removed, because route matching no longer reads the config.

2. Microsecond benchmarks. A measured call of a few microseconds is mostly the fixed cost of entering it plus cold cache misses, which move with code and memory layout. CodSpeed's docs put cache-driven micro-benchmark changes at under 1 µs. pyperf handles this with hardcoded inner_loops (repeating the operation inside the benchmark) and calibrates to at least 100 ms per value. CodSpeed's simulation runs one measured call and ignores pedantic rounds/iterations, so the repetition has to live inside the measured function.

  • Hot path, repeated per measured call. Every one now does at least ~100 µs of uninstrumented work:

    Benchmark Runs on Repeats
    decode_event_headers[4/16/64] every socket connect 200
    state_update_dataclass_serialization every update 1000
    from_event_type handlers returning events 500
    emit_update every update 50
    memory modify / read_only_modify every event ITERATIONS
    scalar mutation, dependency fan-out every event 50, one shared helper
    small wire payloads, edge-case payload every update 10–100
    state_collection_assignment[10/1000/100000] events setting a list 100k items per call, as 100000 // size assignments

    decode_event_headers[0] is dropped, because it measured only the call. The decoded headers are cached per socket (_static_router_data), so decoding happens once per connection; test_on_event_router_data covers the per-event merge.

  • Removed, compile time or once per page: route_argument_extraction, var_arithmetic_chain, var_to_dispatch, var_guess_type_dispatch. The page, compilation and sized var-operation benchmarks cover that code.

  • Removed, exact duplicates:

    • state_manager_memory_warm_get duplicates test_get_state[memory], which already batches the same call.
    • burst_independent_tokens[1] and burst_same_token[1] are the same single warm event as test_process_event_warm.

3. state_collection_assignment: cost depended on interpreter state. Garbage collection isn't the cause: pytest-codspeed runs gc.collect() and gc.disable() around setup and the measured call. Under callgrind the run-to-run difference sat entirely in _Py_dict_lookup (type-attribute cache misses on getattr(cls, "__origin__")) and pymalloc. Both happened once per item in _isinstance, which also allocated five closure cells per call, because generator expressions captured its arguments.

  • The fix, in reflex-base: _isinstance now checks container items through _all_isinstance, which accepts an item whose type is exactly the item class without a call. Var classes are excluded, because _isinstance checks a LiteralVar by the value it wraps. The remaining generator expressions are now loops, so _isinstance has no cell variables.
  • Effect under callgrind: state_collection_assignment[100000] and isinstance_container[list_int] run about 36x fewer instructions; wall time for the first went from 50 ms to 1.6 ms. The other isinstance benchmarks run 1.2–1.5x fewer.

How this was checked. pytest-codspeed's hooks reset and dump callgrind counters per benchmark under valgrind, so stock callgrind gives a close local approximation of CodSpeed's simulation. tests/benchmarks/README.md now documents the command and the sizing rules.

  • Method: each changed benchmark was run alone and again after another module.
  • Batched benchmarks: they now drift 0.0–0.7% between the two runs, down from up to ~2%. decode_event_headers[64] went from 1.98% to 0.28%.
  • The one exception: state_collection_assignment[10] drifts 1.4%. That comes from simulated cache misses on perf-trampoline addresses, the code-layout noise CodSpeed's docs describe; instructions differ by 0.03%.

This is a local approximation: CI on this PR is the real test.

Expected on CodSpeed for this PR: large changes on every benchmark that was batched, rewritten or sped up, and missing results for the removed ones. They need acknowledging once.

Not changed here:

  • wire_serialization[model_1000] (−4.4 to −6.0% on 3 PRs, one JS-only) and state_access[read_cached_computed] (−10% and −15% on 2 PRs) also look noisy. Worth revisiting if they keep flipping after this.
  • CodSpeed's exclude-allocations action input exists in the pinned v5.6.3. It is left off: allocation is a real part of Python hot-path cost, and it isn't documented whether CPython's own pymalloc counts as allocator time.
  • The regression threshold is a project setting outside this repo and is left as is.

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable? test_isinstance_checks_container_items covers 21 container cases with Vars treated both as their type and as values; it passes on the old and new _isinstance.
  • Have you successfully ran tests with your changes locally? Unit tests, ruff and pyright pass. The only failures are the 14 Playwright tests in tests/units/reflex_bench/drivers/test_browser.py, because the local container lacks the matching Chromium headless shell.

https://claude.ai/code/session_01LwJUCWYQL5BocrPjnH5hFX

test_route_matching measured three lru_cache hits: get_router memoizes
each path and CodSpeed calls the benchmark once before measuring it. The
measured call was about 8k instructions with ~360 cold cache misses, so
the route table size did not register and the result flipped by about
5.5% between commits. It now calls the wrapped scan for 100 lookups that
cover the index, a dynamic route, the catch-all, static routes spread
over the table and misses, so the cost grows with the table (1.3M, 11M
and 111M instructions under callgrind, stable to 0.13% across runs).
The cached_rxconfig fixture is removed: route matching no longer reads
the config.

test_process_event_burst_independent_tokens[1] ran the same single
warm-token event as test_process_event_warm.

Claude-Session: https://claude.ai/code/session_01LwJUCWYQL5BocrPjnH5hFX
A measured call of a few microseconds is mostly the fixed cost of entering
it and cold cache misses, so its CodSpeed result moves with unrelated
changes. Benchmarks of code that does not run per event are removed:

- test_decode_event_headers: runs once per socket connection.
- test_route_argument_extraction: runs once per page, at add_page.
- test_var_arithmetic_chain, test_var_to_dispatch,
  test_var_guess_type_dispatch: compile time, and the operation families
  in the same module cover them with sized bodies.
- test_state_update_dataclass_serialization: serialize_state_update is the
  StateUpdate serializer, already measured by every wire benchmark.
- test_state_manager_memory_warm_get: duplicate of test_get_state[memory],
  which batches the same call.

Hot-path benchmarks now repeat their operation per measured call:
from_event_type (500), emit_update (50), memory modify and read-only
modify (ITERATIONS), scalar mutation and dependency fan-out (50, through
a shared helper), small wire payloads and the edge-case payload (10-100
encodes). Every remaining benchmark takes at least ~30 us uninstrumented,
the batched ones at least ~110 us.

Claude-Session: https://claude.ai/code/session_01LwJUCWYQL5BocrPjnH5hFX
Validating a list[int] assigned to a state var called _isinstance once per
item. Each call allocated five closure cells, because generator
expressions in the function captured its arguments, and looked up
__origin__ on the item type. The cost of both depends on interpreter
state left by earlier code (pymalloc pools, the type attribute cache),
which is why state_collection_assignment flipped by 5-14% on CodSpeed
for changes that did not touch it.

The container branches now check items through _all_isinstance, which
accepts an item whose type is exactly the item class without a call, and
falls back to _isinstance otherwise. Var classes are excluded, since
_isinstance checks a LiteralVar by the value it wraps. The remaining
generator expressions are loops, so _isinstance has no cell variables.

Under callgrind, state_collection_assignment[100000] and
isinstance_container[list_int] run about 36x fewer instructions, and
list_optional, list_typeddict and isinstance_scalar 1.2-1.5x fewer.
state_collection_assignment keeps only its 100000 case: the smaller ones
now measure little beyond the fixed cost of one assignment.

Claude-Session: https://claude.ai/code/session_01LwJUCWYQL5BocrPjnH5hFX
It ran the same single event on one warm token as test_process_event_warm.

Claude-Session: https://claude.ai/code/session_01LwJUCWYQL5BocrPjnH5hFX
@FarhanAliRaza
FarhanAliRaza requested review from a team and Alek99 as code owners November 12, 2026 21:21
@greptile-apps

greptile-apps Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The reviewed changes appear safe to merge.

Summary

This PR makes small benchmarks do more work per measured call and speeds up checks of container items.

  • Container type checks skip extra work for items that already match.
  • Route benchmarks now measure scans across varied paths.
  • Fast benchmarks now repeat work and skip duplicate measurements.

No actionable new issues were found in the changes since the previous review.

Reviews (2) · Last reviewed commit: "Restore the hot-path benchmarks removed ..." · Reviewed by Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 12 files

View guided diff | Turn on auto-fix | Re-trigger cubic

@codspeed

codspeed Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 89.71%

1 warning

⚠️ 14 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚡ 12 improved benchmarks
❌ 27 regressed benchmarks
✅ 104 untouched benchmarks
⏩ 8 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_route_matching[1000] 47.5 µs 92,080.5 µs -99.95%
❌ test_state_collection_assignment[10] 311 µs 403,774.7 µs -99.92%
❌ test_route_matching[100] 47.7 µs 9,607.2 µs -99.5%
❌ test_from_event_type[lambda_event_handler] 184.3 µs 31,737.8 µs -99.42%
❌ test_decode_event_headers[64] 164.8 µs 23,309.7 µs -99.29%
❌ test_from_event_type[event_spec] 94.1 µs 11,544.4 µs -99.18%
❌ test_state_update_dataclass_serialization 34.1 µs 3,986.7 µs -99.14%
❌ test_decode_event_headers[16] 67.5 µs 6,106 µs -98.89%
❌ test_from_event_type[event] 47.8 µs 2,515.8 µs -98.1%
❌ test_decode_event_headers[4] 38.4 µs 1,816.6 µs -97.89%
❌ test_state_delta_scalar_mutation[10000] 7.6 ms 353.9 ms -97.84%
❌ test_computed_dependency_fanout[50] 1.6 ms 72.6 ms -97.75%
❌ test_state_delta_scalar_mutation[1000] 1.1 ms 42.3 ms -97.5%
❌ test_computed_dependency_fanout[10] 454.7 µs 15,447.4 µs -97.06%
❌ test_state_delta_scalar_mutation[100] 382.9 µs 10,937.9 µs -96.5%
❌ test_state_update_wire_serialization[scalar_100b] 116.3 µs 3,130.2 µs -96.28%
❌ test_route_matching[10] 50.2 µs 1,265.2 µs -96.03%
❌ test_state_delta_scalar_mutation[10] 324 µs 7,861.6 µs -95.88%
❌ test_computed_dependency_fanout[2] 221 µs 5,015.6 µs -95.59%
❌ test_computed_dependency_fanout[0] 139.9 µs 2,047.5 µs -93.17%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing farhan/upbeat-thompson-ftzgzb (bc739c2) with main (cb9ee91)

Open in CodSpeed

Footnotes

  1. 8 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. ↩

Header decoding runs on every socket connect (page load, new tab,
reconnect), serialize_state_update on every update, and small list
assignments on every event that sets a list var, so their benchmarks
stay, repeating the operation per measured call instead:

- test_decode_event_headers: 200 decodes per call. The 0-header case is
  dropped: it measured only the call. The docstring said per-event; the
  decoded headers are cached per socket.
- test_state_update_dataclass_serialization: 1000 conversions per call.
- test_state_collection_assignment[10] and [1000] are back. Every size
  validates 100,000 items per call, as 100,000 // size assignments, so
  the sizes split the same work between per-assignment and per-item cost.

Under callgrind the restored benchmarks drift 0.0-0.7% between a run
alone and one after another module, except assignment[10] at 1.4%, which
is simulated cache misses on perf trampoline addresses. Before, a header
decode call was 44k-204k estimated cycles and drifted up to 2%.

The README now says how to size a benchmark and how to check it locally.

Claude-Session: https://claude.ai/code/session_01LwJUCWYQL5BocrPjnH5hFX
@FarhanAliRaza FarhanAliRaza added bug Something isn't working perf Performance-improving changes testing labels Nov 13, 2026 — with ChatGPT Codex Connector
@FarhanAliRaza
FarhanAliRaza merged commit 832fcfd into main Nov 13, 2026
174 of 176 checks passed
@FarhanAliRaza
FarhanAliRaza deleted the farhan/upbeat-thompson-ftzgzb branch November 13, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working perf Performance-improving changes testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants