perf: perform shadow variable update exactly once#2305
Merged
triceo merged 5 commits intoMay 14, 2026
Merged
Conversation
There were a couple of inefficiently discovered - When a declarative shadow variable changes, it goes through the same plumbing as other variables. This is wasteful, since by definition, declarative shadow variables cannot affect the structure of the graph, and the graphs already modify the queues directly. - When group alignment is used in a fixed graph, the same entity's update might be called multiple times. This is because the change set is a priority queue, which allows duplicate elements. Now: - We shortcut before/after variable change to return immediately if the graph is currently updating - For fixed graphs, we have a seperate bitset to ensure the same node cannot be added to the changed queue multiple times.
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
Profiling showed calculating the forward edges was taking significant time on some datasets. Sinced fixed graphs cannot change, they can cache each node's forward edges on construction.
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
Before, DefaultTopologicalOrderGraph's `nodeForwardEdges` returned the `componentForwardEdges` instead, creating unneccessary waste and compute.
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
triceo
approved these changes
May 14, 2026
Collaborator
triceo
left a comment
triceo
left a comment
There was a problem hiding this comment.
I like it! LGTM when comments resolved.
I started the microbenchmark suite; I may have some questions after I see the results.
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
|
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
2 participants
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.
There were a couple of inefficiently discovered
When a declarative shadow variable changes, it goes through the same plumbing as other variables. This is wasteful, since by definition, declarative shadow variables cannot affect the structure of the graph, and the graphs already modify the queues directly.
When group alignment is used in a fixed graph, the same entity's update might be called multiple times. This is because the change set is a priority queue, which allows duplicate elements.
Now:
We shortcut before/after variable change to return immediately if the graph is currently updating
For fixed graphs, we have a seperate bitset to ensure the same node cannot be added to the changed queue multiple times.