perf(loops): make For-loop scheduling linear in the item count - #225
Open
lstein wants to merge 1 commit into
Open
perf(loops): make For-loop scheduling linear in the item count#225lstein wants to merge 1 commit into
lstein wants to merge 1 commit into
Conversation
Scheduling a `For` loop was quadratic in its collection size, in two
places:
- `create_for_iteration` deep-copied the whole collection for every
iteration. A 1200-item loop made 1.44M `deepcopy` calls, almost all of
them re-copying items the loop had already passed. The collection now
moves from one prepared `For` node to the next: the caller drops its
reference before handing the list over, so exactly one node owns it at
a time. The assignment goes through `object.__setattr__` because
pydantic's `validate_assignment` would otherwise rebuild the list and
make the move O(n) again — the same idiom `_prepare_iterate_inputs`
already uses for this field.
- Marking a source node complete rescanned every prepared execution node
the source had ever produced, on every completion. `GraphExecutionState`
now keeps a per-source count of prepared nodes that have not executed,
built lazily and maintained at register/execute/drop, so the check is a
dict lookup.
Per-node scheduling cost for a trivial `for` loop, measured with the GC
disabled so the numbers reflect scheduling rather than gen-2 collections:
n=300 n=1200 n=2400 n=4800
before 399 733 1169 -
after 344 338 338 335
Flat out to n=9600. `iterate` was already linear and is unchanged at
~130us/node.
`test_loop_scheduler_overhead_is_linear` guarded this but was itself
flaky on Windows: it measured with `time.process_time`, and Windows
samples process CPU on the ~15.6ms scheduler tick, which quantized the
300-item baseline down to a single tick and made the ratio meaningless
(this is what failed CI on an unrelated frontend PR). It now measures
wall clock and takes the fastest of several samples, since a busy host
only ever makes a run slower.
The existing deep-copy regression test pinned the old behaviour of two
copies per iteration; it now drives a full loop and asserts one copy per
iteration rather than one per collection entry per iteration, which is
what actually distinguishes linear from quadratic. A new test asserts
the pending-prepared counts agree with a full rescan at every scheduling
step.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jcp5TPVjPL469HhuWYT45m
lstein
requested review from
JPPhoto,
Pfannkuchensack and
blessedcoolant
as code owners
September 7, 2026 03:34
Collaborator
Author
|
@JPPhoto Were you working on the collection For-loop handling? This is a small change that converts quadratic behavior into O(n). Could you take a look at it? |
Collaborator
I'm working on refactoring everything, hold off on merging this and maybe I can just incorporate that into my refactor if timing works out - otherwise I'll do a proper review and figure out how this fits in. |
Collaborator
Author
|
Sure. This PR is directed at a flaky pytest that times out on Windows systems. About 1 in 4 PRs initially fail CI because of this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to fix the sporadic pytest failures on
test_loop_scheduler_overhead_is_linear.Scheduling a
Forloop was quadratic in its collection size. This showed up astest_loop_scheduler_overhead_is_linearfailing onpy3.11 windows-cpuin #221 — a frontend-only PR that touches no Python. That test was barely passing on merit: on a quiet Linux machine the 300→1200 ratio it measures came out anywhere between 1.30 and 1.84 against its own 1.5 threshold.The two quadratics
The loop collection was deep-copied per iteration.
_ExecutionMaterializer.create_for_iterationdidnew_node.collection = copydeep(collection)for every iteration, so a 1200-item loop made ~1.44Mdeepcopycalls (n²/2), almost all of them re-copying items the loop had already passed.The collection now moves from one prepared
Fornode to the next: the caller drops its own reference before handing the list over, so exactly one node owns it at a time — which is already the design, since the previous node's collection was being cleared immediately afterwards anyway. The assignment usesobject.__setattr__because pydantic'svalidate_assignmentwould otherwise re-validate and rebuild the list, making the move O(n) again; that is the same idiom_prepare_iterate_inputsalready uses for this field.Source-node completion rescanned every prepared node.
_mark_source_node_completeandmark_exec_node_skippedranall(n in executed for n in prepared_ids)on every completion, andprepared_idsgrows to n for a loop source.GraphExecutionStatenow keeps a per-source count of prepared nodes that have not executed, built lazily and maintained at register / execute / drop, so the check is a dict lookup. The invariant iscounts[s] == |{e in source_prepared_mapping[s] : e not in executed}|.Effect
Per-node scheduling cost for a trivial
forloop, GC disabled so the numbers reflect scheduling rather than gen-2 collections:Flat across a 32× range. Profiling confirms every hot frame's call count now scales exactly 1.00 per node.
iteratewas already linear and is unchanged at ~130µs/node. With the GC on there is still growth past n≈4800, but that is gen-2 collection scanning the accumulating execution graph, which is inherent to retaining executed nodes and results.Tests
test_loop_scheduler_overhead_is_linearmeasured withtime.process_time. Windows samples process CPU on the ~15.6ms scheduler tick, which quantized the 300-item baseline down to a single tick (every number in the CI failure is an exact multiple of 1/64s) and made the ratio meaningless. It now measures wall clock and takes the fastest of several samples, since a busy host only ever makes a run slower. The ratio it now sees is 0.99 for bothforanditerate, against a 1.5 threshold.test_graph_for_iteration_does_not_deep_copy_collection_twicepinned the old behaviour of two copies per iteration. It is replaced bytest_graph_for_iteration_does_not_copy_collection_per_iteration, which drives a full loop and asserts one copy per iteration rather than one per collection entry per iteration — the thing that actually distinguishes linear from quadratic.test_graph_for_scheduling_keeps_prepared_completion_counts_consistentasserts the new counts agree with a full rescan before and after every scheduling step.Verification
Full suite: 6630 passed, 0 failed. (Three
tests/backend/pidCUDA bf16 tolerance tests fail identically on unmodifiedmainand do not run on CI's CPU runners.)An adversarial review of the diff attacked both changes — every mutation site of
executedand the prepared-node mappings repo-wide, cache staleness acrossmodel_copy/model_validate/deepcopy, dropped-then-executed prepared ids, nested For and For-in-Iterate, error paths, and JSON round-trips — with a differential fuzz harness (102 graph/round-trip combinations) that produced byte-identical traces againstHEAD. It found no correctness break, and three real issues that are fixed here:validate_assignmentrebuilt the list — henceobject.__setattr__.copydeep, and nothing in the scheduler mutates a collection entry in place — but it is now documented rather than accidental._reset_runtime_caches()reset every other derived private attr but not the new counts. Inert today (its only caller runs right after pydantic nulls private attrs), but a future caller would have got a count that never reaches 0, so the source would never be marked complete and the session would never finish.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jcp5TPVjPL469HhuWYT45m