Add FrozenVector to wrap neighbors

[simonschoelly]

This PR makes the output of neighbors, inneighbors and outneighbors immutable by wrapping the returned Vector in an immutable wrapper called FrozenVector.

I noticed that the generated machine code for Base.getindex is slightly different (one additional instruction) with FrozenVector compared to the previously used Vector - this does probably not have a significant performance impact, but I should run some benchmarks.

See also this issue: #316

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.28%. Comparing base (2bc509d) to head (3dc9701).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/frozenvector.jl	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
- Coverage   97.32%   97.28%   -0.04%     
==========================================
  Files         125      126       +1     
  Lines        7652     7662      +10     
==========================================
+ Hits         7447     7454       +7     
- Misses        205      208       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gdalle]

We could wait until #315 is merged to have a proper benchmarking pipeline?

[simonschoelly]

We could wait until #315 is merged to have a proper benchmarking pipeline?

That one has been merged now.

[gdalle]

It has been merged to a branch of the main repo (benchx) so that @filchristou could experiment with CI setups and actions outside of a fork. It's not yet on master, and more importantly even once the pipeline is set up we still won't have a satisfying set of tasks to benchmark.

Besides performance issues, I am personally opposed to this change, because the additional wrapper might be confusing for users (what is this FrozenVector thing and what can I do with it?) and incompletely implemented (what if we forget a useful vector operation?). Also I think there is a case for considering such a change breaking

[simonschoelly]

It has been merged to a branch of the main repo (benchx) so that @filchristou could experiment with CI setups and actions outside of a fork. It's not yet on master, and more importantly even once the pipeline is set up we still won't have a satisfying set of tasks to benchmark.

Right, I had just looked at the merged status.

Besides performance issues, I am personally opposed to this change, because the additional wrapper might be confusing for users (what is this FrozenVector thing and what can I do with it?) and incompletely implemented (what if we forget a useful vector operation?). Also I think there is a case for considering such a change breaking

Nowhere do we guarantee that we run an Vector. From the documentation:

  inneighbors(g, v)

  Return a list of all neighbors connected to vertex v by an incoming edge.

  Implementation Notes
  ––––––––––––––––––––

  Returns a reference to the current graph's internal structures, not a copy. Do not modify result. If the
  graph is modified, the behavior is undefined: the array behind this reference may be modified too, but
  this is not guaranteed.

And FrozenVector is even an AbstractVector although we also do not guarantee that by the documentation. So why should one use any other feature of a Vector and why should we support that?
On the other hand, we are able to prevent users from making a lot of hard to detect errors when they - for some -reason - try to modify that result without making a copy.

And I don't think performance should be affected seriously.

[simonschoelly]

Futhermore, if someone is actually confused what a FrozenVector is, they can simply consult the documentation.

[gdalle]

Okay, I'm convinced that the benefits outweigh the costs on the correctness side.

Can you maybe run some benchmarks locally, say Dijkstra on a large enough graph, to see if there is a big performance hit from creating the wrappers every time? Maybe even degree(g) could suffer a bit.

[filchristou]

I will probably merge to master before the community call. After you can use that for benchmarks and talk further.

[gdalle]

Awesome, thank you so much! Don't hesitate to ask for a review beforehand though, to follow ColPrac

[Krastanov]

this needs a reformatting, but otherwise seems good to go

[Krastanov]

Given Guillaume's comment, I will wait for the cleaned up benchmark runner before I merge this (see #490 )

[github-actions]

Benchmark Results (Julia v1)

Time benchmarks

	master	3dc9701...	master / 3dc9701...
centrality/digraphs/betweenness_centrality	17.1 ± 0.45 ms	16 ± 0.67 ms	1.07 ± 0.053
centrality/digraphs/closeness_centrality	12 ± 0.67 ms	11.3 ± 0.4 ms	1.06 ± 0.071
centrality/digraphs/degree_centrality	1.94 ± 0.17 μs	2.04 ± 0.17 μs	0.951 ± 0.11
centrality/digraphs/katz_centrality	0.871 ± 0.046 ms	0.861 ± 0.058 ms	1.01 ± 0.086
centrality/digraphs/pagerank	0.0364 ± 0.00052 ms	0.0364 ± 0.00059 ms	0.999 ± 0.022
centrality/graphs/betweenness_centrality	28.8 ± 1.4 ms	29.1 ± 1.2 ms	0.992 ± 0.062
centrality/graphs/closeness_centrality	21.7 ± 0.41 ms	21.8 ± 0.57 ms	0.996 ± 0.032
centrality/graphs/degree_centrality	1.5 ± 0.18 μs	1.54 ± 0.17 μs	0.973 ± 0.16
centrality/graphs/katz_centrality	1.02 ± 0.045 ms	1.03 ± 0.062 ms	0.995 ± 0.074
connectivity/digraphs/strongly_connected_components	0.0433 ± 0.0011 ms	0.0428 ± 0.001 ms	1.01 ± 0.035
connectivity/graphs/connected_components	24.2 ± 0.67 μs	24.5 ± 0.71 μs	0.987 ± 0.04
core/edges/digraphs	7.78 ± 0.7 μs	7.02 ± 0.019 μs	1.11 ± 0.1
core/edges/graphs	17.4 ± 0.081 μs	17 ± 0.089 μs	1.02 ± 0.0071
core/has_edge/digraphs	5.31 ± 0.36 μs	5.26 ± 0.34 μs	1.01 ± 0.095
core/has_edge/graphs	5.56 ± 0.38 μs	5.55 ± 0.36 μs	1 ± 0.095
core/nv/digraphs	0.361 ± 0.01 μs	0.361 ± 0.01 μs	1 ± 0.039
core/nv/graphs	0.371 ± 0.01 μs	0.391 ± 0.01 μs	0.949 ± 0.035
edges/fille	8.31 ± 0.96 μs	7.75 ± 0.92 μs	1.07 ± 0.18
edges/fillp	5.3 ± 4 μs	4.68 ± 4 μs	1.13 ± 1.3
edges/tsume	2.69 ± 0.14 μs	2.54 ± 0.17 μs	1.06 ± 0.09
edges/tsump	2.62 ± 0.02 μs	2.67 ± 0.04 μs	0.982 ± 0.016
insertions/SG(n,e) Generation	25 ± 3.7 ms	24.8 ± 3.4 ms	1.01 ± 0.2
parallel/egonet/twohop	0.313 ± 0.012 s	0.281 ± 0.0024 s	1.11 ± 0.042
parallel/egonet/vertexfunction	2.44 ± 0.39 ms	2.2 ± 0.056 ms	1.11 ± 0.18
serial/egonet/twohop	0.317 ± 0.011 s	0.284 ± 0.00091 s	1.12 ± 0.04
serial/egonet/vertexfunction	2.46 ± 0.37 ms	2.19 ± 0.053 ms	1.12 ± 0.17
traversals/digraphs/bfs_tree	0.0498 ± 0.0022 ms	0.0493 ± 0.0019 ms	1.01 ± 0.059
traversals/digraphs/dfs_tree	0.0639 ± 0.0023 ms	0.0634 ± 0.0024 ms	1.01 ± 0.052
traversals/graphs/bfs_tree	0.053 ± 0.0019 ms	0.053 ± 0.0015 ms	1 ± 0.046
traversals/graphs/dfs_tree	0.066 ± 0.0021 ms	0.0652 ± 0.0018 ms	1.01 ± 0.042
time_to_load	0.538 ± 0.0057 s	0.53 ± 0.0039 s	1.01 ± 0.013

Memory benchmarks

	master	3dc9701...	master / 3dc9701...
centrality/digraphs/betweenness_centrality	0.29 M allocs: 24 MB	0.29 M allocs: 24 MB	1
centrality/digraphs/closeness_centrality	18.6 k allocs: 14.5 MB	18.6 k allocs: 14.5 MB	1
centrality/digraphs/degree_centrality	8 allocs: 5.01 kB	8 allocs: 5.01 kB	1
centrality/digraphs/katz_centrality	2.63 k allocs: 2.83 MB	2.63 k allocs: 2.83 MB	1
centrality/digraphs/pagerank	21 allocs: 14.9 kB	21 allocs: 14.9 kB	1
centrality/graphs/betweenness_centrality	0.545 M allocs: 0.0313 GB	0.545 M allocs: 0.0313 GB	1
centrality/graphs/closeness_centrality	19.3 k allocs: 14 MB	19.3 k allocs: 14 MB	1
centrality/graphs/degree_centrality	10 allocs: 5.43 kB	10 allocs: 5.43 kB	1
centrality/graphs/katz_centrality	2.96 k allocs: 3.1 MB	2.96 k allocs: 3.1 MB	1
connectivity/digraphs/strongly_connected_components	1.05 k allocs: 0.075 MB	1.05 k allocs: 0.075 MB	1
connectivity/graphs/connected_components	0.061 k allocs: 22.5 kB	0.061 k allocs: 22.5 kB	1
core/edges/digraphs	3 allocs: 0.0938 kB	3 allocs: 0.0938 kB	1
core/edges/graphs	3 allocs: 0.0938 kB	3 allocs: 0.0938 kB	1
core/has_edge/digraphs	20 allocs: 12.6 kB	20 allocs: 12.6 kB	1
core/has_edge/graphs	28 allocs: 13.8 kB	28 allocs: 13.8 kB	1
core/nv/digraphs	3 allocs: 0.0938 kB	3 allocs: 0.0938 kB	1
core/nv/graphs	3 allocs: 0.0938 kB	3 allocs: 0.0938 kB	1
edges/fille	3 allocs: 0.153 MB	3 allocs: 0.153 MB	1
edges/fillp	3 allocs: 0.153 MB	3 allocs: 0.153 MB	1
edges/tsume	0 allocs: 0 B	0 allocs: 0 B
edges/tsump	0 allocs: 0 B	0 allocs: 0 B
insertions/SG(n,e) Generation	0.0465 M allocs: 10.9 MB	0.0465 M allocs: 10.9 MB	0.999
parallel/egonet/twohop	10 allocs: 0.0768 MB	10 allocs: 0.0768 MB	1
parallel/egonet/vertexfunction	10 allocs: 0.0768 MB	10 allocs: 0.0768 MB	1
serial/egonet/twohop	3 allocs: 0.0764 MB	3 allocs: 0.0764 MB	1
serial/egonet/vertexfunction	3 allocs: 0.0764 MB	3 allocs: 0.0764 MB	1
traversals/digraphs/bfs_tree	2.34 k allocs: 0.113 MB	2.34 k allocs: 0.113 MB	1
traversals/digraphs/dfs_tree	2.44 k allocs: 0.118 MB	2.44 k allocs: 0.118 MB	1
traversals/graphs/bfs_tree	2.52 k allocs: 0.121 MB	2.52 k allocs: 0.121 MB	1
traversals/graphs/dfs_tree	2.63 k allocs: 0.127 MB	2.63 k allocs: 0.127 MB	1
time_to_load	0.145 k allocs: 11 kB	0.145 k allocs: 11 kB	1

[Krastanov]

None of the benchmarks show slowdown (not that the benchmarks are super thorough). I will merge this after the tests pass, given that the review from a few years ago seems to have been positive, even if somewhat muted. On skimming it quickly today, I also do not see issues with this approach, and it seems like a good protection against footguns.

[Krastanov]

two doctests fail in an expected fashion, will submit a patch