Rewrote A* to be faster with fewer allocations.

[BorisIvanovic]

Rewrote A* to more closely follow widely-adopted implementations, greatly speeding up its runtime (roughly 200x faster) and reducing its memory usage (roughly 300x fewer allocations and size of allocations).

[codecov]

Codecov Report

Merging #1231 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
+ Coverage   99.62%   99.67%   +0.04%     
==========================================
  Files          94       94              
  Lines        4311     4323      +12     
==========================================
+ Hits         4295     4309      +14     
+ Misses         16       14       -2

[sbromberger]

This looks pretty solid! Can you post some benchmarks? Where did you find the performance gains?

[BorisIvanovic]

Thanks! Here's the benchmark I ran using the following graph:

graph = barabasi_albert(1000, 100, 100, seed=15)
@profile (for i=1:1000; a_star(graph, 1, 1000); end)

And here are the results I got:

OLD

BenchmarkTools.Trial: 
  memory estimate:  331.34 MiB
  allocs estimate:  8710892
  --------------
  minimum time:     2.251 s (14.14% GC)
  median time:      2.319 s (17.01% GC)
  mean time:        2.320 s (16.80% GC)
  maximum time:     2.392 s (19.10% GC)
  --------------
  samples:          3
  evals/sample:     1

NEW

BenchmarkTools.Trial: 
  memory estimate:  293.77 KiB
  allocs estimate:  8764
  --------------
  minimum time:     1.419 ms (0.00% GC)
  median time:      1.664 ms (0.00% GC)
  mean time:        1.824 ms (3.26% GC)
  maximum time:     75.829 ms (97.60% GC)
  --------------
  samples:          2716
  evals/sample:     1

So maybe a bit better than what I reported above :P

As for where we got the improvement, there were two major areas:

1. Array concatenation new_path = cat(path, E(u, v), dims=1) (line 26 of astar.jl) is very slow. That actually eats up the majority of the runtime before.
2. Hashing an ever-increasing array enqueue!(frontier, ... (lines 28-31 of astar.jl) is very slow.

Both of these can be seen in the attached ProfileView graph of the old method (the big red pyramid on the left is the concatenation, the towers on the right are priority queue hashing):

whereas our new implementation has its runtime cost spread out over many different underlying functions:

Unfortunately, I can't upload the actual SVG outputs, but you could obtain these by running

Profile.clear()
@profile (for i=1:1000; a_star(graph, 1, 1000); end)

(just know that for the old method, you'll have to change the 1000 to a 1 otherwise Profile's internal buffer will saturate).

[sbromberger]

I really appreciate your attention to detail in the code and the benchmarks. Once we have tests passing (not your fault; we have randomness in a few functions that causes failures sometimes) I'm happy to merge this. Thank you.

[simonschoelly]

This happened quite fast, so I don't know if there is a point in still doing a code review.

One thing that I noticed is, that the result type will change from Vector{Edge{eltype(g)}} to Vector{edgetype{g}}. This is the same for SImpleGraph and SimpleDiGraph, but not necessarily for other graphs. I don't know if the constructor edgetype(g)(u, v) works for all graphs (What if there is metadata on an edge?)

Also, when doing benchmarks, one should maybe also do some with non-constant edge weights and a non-constant heuristic.

For profiling, I think there is a command, that can increase the number of samples collected.

[sbromberger]

Anything that gives us a 3 orders of magnitude speedup on SimpleGraphs is a no-brainer to me.

As far as the return value goes, my opinion is that the latter (edgetype(g)) is more correct: it's part of the API, and since any graph g must have edges of edgetype(g), this function should return structures that are valid edges of g. Edge{eltype(g)} might not always be a valid edgetype for g. I can't think of am example now where this isn't the case, but even if there are no such examples, edgetype(g) is a fair - and more appropriate - substitute.

If there is metadata on the edge, it should have reasonable default values. The algorithm does not claim to create or preserve metadata of the returned edges. (Perhaps we should consider a make_edge_from(u, v, e::Edge) function, but that's outside the scope of this discussion and it seems pointless to copy metadata into a list of edges that represent a path.)

As far as benchmarks, you're right that more formal benchmarking is nice, but I have not seen that in many PRs; in fact, this is some of the most thorough benchmarking and analysis I've seen for any PR in recent memory and would like to encourage this sort of output.

If there is a specific objection to the code – if you've found a regression – then I'm all ears and we should revisit. But as it stands, we've got a massive improvement in speed and it passes all our current tests.

[simonschoelly]

Apologies, if that came over as some harsh criticism, that was never the intention and the comment on the benchmark should be seen more as some kind of feedback, as it should be clear from the code changes, that this code will run faster.

There are two problems with using edgetype(g). The first is, that depending on the graph we might waste a lot of time. Using Edge{eltype(g)} does of course also use twice as much space as necessary, but at least not more, and the taken edges can easily be reconstructed (at long as the graph is not a multigraph at least).

The second one is, that the algorithm uses the constructor edgetype(g)( u, v) which does exist for SimpleGraphs but not necesarily for others (and in my opinion also should not necessarily). The following code did work before, but does crash now, so this will be a breaking change:

using LightGraphs, SimpleWeightedGraphs

A = Float64[0 1; 1 0]
g = SimpleWeightedGraph(A)

a_star(g, 1, 2)

I can create a PR to change that to the old behaviour.

[sbromberger]

MHO is that the problem is not with this code, it is with the fact that SimpleWeightedEdge does not have a constructor SimpleWeightedEdge{T, U}(u::T, v::T, w::U=one(U)). That's probably the correct fix, and we should require any custom edges to be able to be constructed with two integers.

[sbromberger]

This fixes it for SWG:

SimpleWeightedEdge{T,U}(u::T,v::T) where {T<:Integer, U<:Real} = SimpleWeightedEdge(u, v, one(U))