Skip to content
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

Make dag_walk::heads_ok() terminate early in common cases #3950

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Make dag_walk::heads_ok() terminate early in common cases #3950

merged 2 commits into from
Jun 24, 2024

Conversation

martinvonz
Copy link
Member

@martinvonz martinvonz commented Jun 24, 2024 •
edited
Loading

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

yuja
yuja approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

I've added a similar workaround to topo_order_reverse_lazy, but it's timestamp-based, and the current heads_ok() interface wouldn't be compatible with that. I think BFS is also good.

lib/src/dag_walk.rs Outdated Show resolved Hide resolved
lib/src/dag_walk.rs Outdated Show resolved Hide resolved
@martinvonz martinvonz enabled auto-merge (rebase) June 24, 2024 19:17
@martinvonz
dag_walk: heads: rename reachable to heads
a567694 
The name `reachable` feels backwards to me since it keeps track of
inputs that are not reachable from other inputs.
@martinvonz
dag_walk: heads: use bfs and terminate if frontier has single item
b164344 
We use `heads_ok()` for finding the head operations when there are
multiple current op heads. The current DFS-based algortihm needs to
always walk all the way to the root. That can be expensive when the
operations are slow to retrieve. In the common case where there are
two operations close to each other in the graph, we should be able to
terminate the search once we've reached the common ancestor. This
patch replaces the DFS by a BFS and adds the early termination.
@martinvonz martinvonz merged commit dddba46 into main Jun 24, 2024
29 checks passed
@martinvonz martinvonz deleted the push-swmwpwnsotty branch June 24, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja 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.
2 participants
@martinvonz @yuja