Skip to content

perf: optimize the updater used for shadow variables in certain cases #1659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 26, 2025
Merged

perf: optimize the updater used for shadow variables in certain cases #1659

merged 7 commits into from
Jun 26, 2025

Conversation

Christopher-Chianelli
Copy link
Contributor

  • If only one entity has declarative variables, and all sources are either of the form "previous.*", "undirectional" or "declarative", then create a cascading updater using next to get successor and (index, id(entity)) for the initial sort.
  • If only one entity has declarative variables, and all sources are either of the form "next.*", "undirectional" or "declarative", then create a cascading updater using previous to get successor and (-index, id(entity)) for the initial sort.
  • If there are no dynamic edges, use a fixed topological order for each variable.
  • Otherwise use the current approach.

Undirectional mean variables that don't create edges in the graph (such as accessing a fact on the inverse or genuine variables).

@Christopher-Chianelli
perf: add specialized variable reference graph when all edges are fixed
c2c75b8
@Christopher-Chianelli
perf: Use a simplified updater when there is only a single direction …
72b5795 
…an update can go

- If only one entity has declarative variables, and all sources are either of the form "previous.*", "undirectional" or "declarative", then create a cascading updater using next to get successor and (index, id(entity)) for the initial sort.
- If only one entity has declarative variables, and all sources are either of the form "next.*", "undirectional" or "declarative", then create a cascading updater using previous to get successor and (-index, id(entity)) for the initial sort.
- If there are no dynamic edges, use a fixed topological order for each variable.
- Otherwise use the current approach.

Undirectional mean variables that don't create edges in the graph (such as accessing a fact on the inverse or genuine variables).
triceo
triceo reviewed Jun 24, 2025
View reviewed changes
Copy link
Collaborator

@triceo triceo left a comment

Choose a reason for hiding this comment

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

First round of review complete. In general, we need more comments/explainers.
I'll run the code through some benchmarks now, see what comes up.

@triceo triceo added this to the v1.24.0 milestone Jun 24, 2025
@triceo triceo linked an issue Jun 24, 2025 that may be closed by this pull request
Performance regression in shadow variable processing #1630
Closed
@triceo triceo removed this from the v1.24.0 milestone Jun 24, 2025
triceo
triceo approved these changes Jun 26, 2025
View reviewed changes
Copy link
Collaborator

@triceo triceo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The improvements are impressive!
LGTM after comments are resolved.
Also please resolve the Sonar issues - the vast majority of them are trivial, and make sense.

@Christopher-Chianelli @triceo
chore: logging
bdbd3df 
Co-authored-by: Lukáš Petrovický <lukas@petrovicky.net>
@triceo triceo merged commit 81374a6 into TimefoldAI:main Jun 26, 2025
43 checks passed
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
91.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@triceo triceo triceo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance regression in shadow variable processing
2 participants
@Christopher-Chianelli @triceo