Skip to content

perf: fixed graph update once - #2673

Draft
fodzal wants to merge 3 commits into
TimefoldAI:mainfrom
fodzal:perf/fixed-graph-update-once
Draft

fodzal wants to merge 3 commits into
TimefoldAI:mainfrom
fodzal:perf/fixed-graph-update-once

Conversation

@fodzal

@fodzal fodzal commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Issue.
In a fixed variable reference graph, when two shadow variables share a source and one
also depends on the other, the shared source enqueues both directly, and updating the first
re-enqueues the second through their edge before dedup catches it. Its supplier then runs twice for
nothing:

@ShadowVariable(supplierName = "durationSupplier") // @ShadowSources("value")
Integer duration;

@ShadowVariable(supplierName = "endTimeSupplier")  // @ShadowSources({ "value", "duration" })
Integer endTime;

value marks duration and endTime directly; duration also marks endTime via the
duration → endTime edge. endTimeSupplier runs twice for the same pass.

Fix. The dedup check happened at enqueue time (visited, seeded from only the first queued
node) instead of at dequeue time. Track updated nodes and check when a node comes out of the
queue instead of when it goes in:

if (updated.get(changedNodeId)) {
    continue;
}

Benchmark. 200 entities, each a chain of 20 shadow ints all sourced from the same genuine
variable and each also depending on the previous link; every move marks all 20 directly and
re-marks 19 of them through the chain:

before after
supplier calls / move ~35 20 (exact)
move time, trivial supplier ~3.4 µs ~3.5 µs (noise; overhead dominates)
move time, realistic supplier ~28.3 µs ~18.6 µs (-34%)

fodzal and others added 3 commits September 17, 2026 12:50
A fixed graph keeps its change set in a plain priority queue, which holds the
same node twice, and innerUpdateChanged() never checks the node it polls. Two
things put duplicates in there: the constructor queues every node without
setting isChanged, so the first markChanged() queues it again; and the pass
tracks which nodes it has queued rather than which ones it has updated, seeded
with the first marked node only, so a node marked before the pass is queued
again once a node ordered before it changes. Its suppliers then run twice.

Track the updated nodes instead and skip a node that is polled twice. The nodes
are polled in topological order, so everything an update can reach is polled
after it and one update per node is always enough.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Diamond" named the value/duration/endTime dependency shape, but only
duration and endTime are graph nodes; value is a genuine variable, so
it never becomes one. The actual graph is a single edge, not a diamond.

Rename to what the domain demonstrates: duration and endTime both
source from value directly, and endTime also sources from duration.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Benchmarking the previous commit surfaced a regression it introduced: the old
code returned immediately when changeTracker was empty, before allocating
visited. The new updated BitSet was allocated unconditionally, even on a pass
with nothing to do, which the "dirty" flag in VariableSupport can trigger for
a move that never touches this graph at all.

Restore the early return. Confirmed on a chain of 20 shared-source shadow
variables across 200 entities: no-op passes go from roughly 155-160ns back to
roughly 100ns, matching the pre-fix baseline, while the real fix still cuts
supplier calls from ~35 to exactly 20 per move on the same domain.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@fodzal
fodzal requested a review from a team September 17, 2026 13:29
@fodzal fodzal changed the title Perf/fixed graph update once perf: fixed graph update once Sep 17, 2026
@triceo
triceo removed their request for review September 17, 2026 13:32
@fodzal
fodzal marked this pull request as draft September 21, 2026 08:00

@Christopher-Chianelli Christopher-Chianelli 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.

There is a simpler way of doing this; modify the original code to use the field isChanged (which already have all nodeIds in changeTracker and thus will not readd it to the queue) and delete the local variable visited. This would eliminate the need of the updated.get call in the loop and will additionally prevent allocating a new bitset on every update. You may need to modify the constructor so isChanged is updated when nodes are added to changeTracker.

This branch was successfully deployed

1 active deployment
external 0401b553 Deployed Sep 17, 2026 by fodzal via approval_required #4830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants