Skip to content

Commit

Permalink
[internal] Clarify graph edge removal (pantsbuild#13975)
Browse files Browse the repository at this point in the history
The docs guarantee the behavior of `remove_edge`, and so we can safely remove edges in reverse order without creating and consuming an `Edges` iterator for each removal.

AFAICT, this will be vaguely cheaper (since `impl Iterator for Edges` [is non-trivial](https://docs.rs/petgraph/0.5.1/src/petgraph/graph_impl/mod.rs.html#1611)), but only in terms of constant factors. The total runtime of edge removal is still significant because `remove_edge` itself is `O(e)`, where `e` is the number of edges connected to four different nodes.

But in the end, the primary advantage of this change is making the comment less vague.

[ci skip-build-wheels]
  • Loading branch information
stuhood authored Dec 28, 2021
1 parent aa79833 commit 22c566e
Showing 1 changed file with 9 additions and 7 deletions.
16 changes: 9 additions & 7 deletions src/rust/engine/graph/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,15 +633,17 @@ impl<N: Node> Graph<N> {
entry.cleaning_failed()
}

// Otherwise, clear the deps.
// NB: Because `remove_edge` changes EdgeIndex values, we remove edges one at a time.
while let Some(dep_edge) = inner
// Otherwise, clear the deps. We remove edges in reverse index order, because `remove_edge` is
// implemented in terms of `swap_remove`, and so affects edge ids greater than the removed edge
// id. See https://docs.rs/petgraph/0.5.1/petgraph/graph/struct.Graph.html#method.remove_edge
let mut edge_ids = inner
.pg
.edges_directed(entry_id, Direction::Outgoing)
.next()
.map(|edge| edge.id())
{
inner.pg.remove_edge(dep_edge);
.map(|e| e.id())
.collect::<Vec<_>>();
edge_ids.sort_by_key(|id| std::cmp::Reverse(id.index()));
for edge_id in edge_ids {
inner.pg.remove_edge(edge_id);
}
}

Expand Down

0 comments on commit 22c566e

Please sign in to comment.