NonEmpty graphs optimizations

[nobrakal]

Hi,

As discussed in #93 , here are several optimizations to Algebra.Graph.NonEmpty.

Mainly with:

• SPECIALIZE pragma when possible and useful
• RULES when possible and useful
• Previous improvements founds while working on Replacing Algebra.Graph by Maybe (Algebra.Graph.NonEmpty) #90

The benchmarks:

vertexList1: 0.08 (good)
vertexCount: 0.07 (good)
hasVertex: 3.71 (bad)
edgeCount: 0.36 (good)
edgeList: 0.37 (good)
hasEdge: 0.25 (good)
hasSelfLoop: 1.02 (OK)
addEdge: 1.00 (OK)
addVertex: 1.02 (OK)
removeVertex1: 1.01 (OK)
equality: 0.34 (good)
removeEdge: 0.96 (OK)
transpose: 1.00 (OK)
dff: 1.01 (OK)
topSort: 1.01 (OK)
edges1: 0.83 (good)

So everything is green, the drop of hasVertex as already been documented here: #90 (comment)

[snowleopard]

I would drop edgeTuple -- I don't think duplicating uncurry edge is such a big deal.

[snowleopard]

Since you now provide much faster implementations compared to the default ones in ToGraph, you should use them in the instance ToGraph declaration.

[nobrakal]

Writing this improvement in the ToGraph instance drop a the performances:
Alga is the improvement moved in the ToGraph instance, and AlgaOld is the current state of this PR.

Mesh

	100
Alga	6.383 μs
AlgaOld	5.100 μs

Clique

	100
Alga	716.0 μs
AlgaOld	170.4 μs

I am not sure why...

I think this is because I can't use NonEmpty graphs in ToGraph, so I can 't use Algebra.Graph.NonEmpty.Vertex but only Algebra.Graph.Vertex, and thus, in the final step, I can only use foldg instead of foldg1.

Also, because we are not sure that induce is removing empty leaves, we loose the fast hasSelfLoop (because it was defined in ToGraph as hasSelfLoop x = hasEdge x x

[snowleopard]

@nobrakal I meant this:

instance ToGraph NonEmpty where
    ...
    hasEdge = hasEdge -- use faster implementation instead of the default one

Surely this can't reduce the performace of hasEdge itself!

[snowleopard]

But users of the typeclass might get the benefit.

So, the rule is: if you use the default implementation, i.e. x = T.x then the instance declaration uses default methods. But otherwise you need to redefine default instance methods, i.e. add x = x to the instance.

[nobrakal]

Oh yes, it is done now :)

[snowleopard]

@nobrakal Thanks, I've added a couple of comments.

vertexList1: 0.08 (good)
vertexCount: 0.07 (good)

Amazing! For these cases we really should be saying too good to be true instead of just good ;-)

[snowleopard]

Presumably, foldg1 AM.vertex AM.overlay AM.connect and foldg1 AIM.vertex AIM.overlay AIM.connect will appear in the instance ToGraph, so you could simply write toAdjacencyIntMap here?

[nobrakal]

Presumably, foldg1 AM.vertex AM.overlay AM.connect and foldg1 AIM.vertex AIM.overlay AIM.connect will appear in the instance ToGraph

You are right for edgeList, it is a mistake.

For edgeIntList there is only adjacencyIntMap in ToGraph, so I cannot use it for AM.edgeSet since adjacencyIntMap only return a IntMap IntSet.

I can add toAdjacencyIntMap in ToGraph if you want.

[snowleopard]

Thanks, you are right, this seems to work only with edgeList.

[nobrakal]

@snowleopard Ahaha, yes this is great :)

Thanks for the comments, I will fix address them shortly.

The travis build is failing here because I just added some missing algorithms to the bench suite, and now the suite take too much time to run. Since the added algorithms are based on their counterpart from AdjacencyMap, I will drop them. A PR is coming soon to fix that !

[snowleopard]

@nobrakal Thanks, merged!