slightly improve performance of getting graph size

[mitchellwrosen]

Just something small I noticed: we got the graph size by getting the size of union(outgoing, incoming), but it's also available via the size of levels.

[mitchellwrosen]

Hrm, sorry, this doesn't seem correct - I'm now noticing combinators like

-- | Remove all the edges that connect the given vertex to its predecessors.
clearPredecessors :: (Eq v, Hashable v) => v -> Graph v e -> Graph v e
clearPredecessors x g@Graph{..} = g
    { outgoing = foldr ($) outgoing
        [ Map.adjust (Map.delete x) z | (z,_) <- getIncoming g x ]
    , incoming = Map.delete x incoming
    }

that don't keep outgoing/incoming in sync with levels.

There seem to be only two: clearPredecessors and collectGarbage. My gut instinct is that those functions ought to keep levels in sync with outgoing/incoming, or at least document why it's okay and intentional not to.

[HeinrichApfelmus]

Just something small I noticed: we got the graph size by getting the size of union(outgoing, incoming), but it's also available via the size of levels.

🤔 I think that this observation is correct in principle. In other words, I would agree that the invariant

Map.keys (outgoing `Map.union` incoming) == Map.keys levels

should hold. If you proceed with this, could you add this invariant as a comment to the Graph type?

• On the matter of clearPredecessors: I think it's correct for it to not adjust the levels — because only edges are removed, but not vertices. I.e. the invariant above should hold.

• On the matter of collectGarbage: The documentation says

  -- | Apply deleteVertex to all vertices which are not predecessors
  -- of any of the vertices in the given list.

  Now, deleteVertex does adjust the levels by calling clearLevels , but collectGarbage fails to do that. I think that's a mistake on the part of collectGarbage, it should restrict the keys of the levels Map to reachables as well.