Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various bugs in matrix graph #427

Merged
merged 5 commits into from May 16, 2021
Merged

Conversation

ABorgna
Copy link
Member

@ABorgna ABorgna commented May 12, 2021

I made multiple changes and fixes to MatrixGraph:

  1. The routine for extending the adjacency matrix for directed graphs created a new matrix and tried to copy the data doing a mem::swap of references to slices, which was effectively a no-op so the adjacencies were lost.
    I changed it to inplace by-element slice::swaps which surprisingly seem to be faster than what was there before. This fixes Edge disappearing, or misunderstanding matrix graph? #425.
    I tried a version that uses an unsafe ptr::swap_nonoverlapping when possible in ABorgna@877edbd and it had around 15% better performance in resize-heavy tests.

  2. Many functions didn't check the adjacency matrix capacity before looking for a node data.
    This was masked in the tests due to new always reserving a 4x4 matrix.

  3. I renamed the new capacity parameter in the extension functions to avoid confusions (was min_node).

  4. I made with_capacity reserve the exact value passed instead of rounding to the next power of two, to avoid quadratic wastage.

Benchmarks:

 name                               base ns/iter  fixed ns/iter  diff ns/iter   diff %  speedup
 add_100_edges_to_self              426           282                    -144  -33.80%   x 1.51
 add_100_nodes                      19,950        12,841               -7,109  -35.63%   x 1.55
 add_5_edges_for_each_of_100_nodes  1,060         904                    -156  -14.72%   x 1.17
 add_adjacent_edges                 27,491        24,965               -2,526   -9.19%   x 1.10
 add_edges_from_root                27,372        25,196               -2,176   -7.95%   x 1.09
 bigger_edges_in                    35            36                        1    2.86%   x 0.97
 bigger_edges_out                   31            35                        4   12.90%   x 0.89
 bigger_kosaraju_sccs               5,272         5,083                  -189   -3.58%   x 1.04
 bigger_neighbors_in                36            36                        0    0.00%   x 1.00
 bigger_neighbors_out               32            35                        3    9.38%   x 0.91
 bigger_tarjan_sccs                 2,779         2,731                   -48   -1.73%   x 1.02
 full_edges_in                      14            15                        1    7.14%   x 0.93
 full_edges_out                     13            13                        0    0.00%   x 1.00
 full_kosaraju_sccs                 953           991                      38    3.99%   x 0.96
 full_neighbors_in                  14            15                        1    7.14%   x 0.93
 full_neighbors_out                 13            13                        0    0.00%   x 1.00
 full_tarjan_sccs                   435           442                       7    1.61%   x 0.98

- Extension function for directed graphs did not copy the edges
- Off by one in with_capacity
- Check if the lazy adjacency matrix contains node before accesing
Avoids unnecessary quadratic memory waste.
Dynamic expansions continue rounding to the next power of two.
Speeds up edge inserting benchmarks by ~35%
Overlapping slices still use the iterative method.
@ABorgna
Copy link
Member Author

ABorgna commented May 12, 2021

Here is the benchmark diff for the unsafe copying vs the iterative version. I think I'll merge that change here.

 name                               fixed ns/iter  fixed_unsafe ns/iter  diff ns/iter   diff %  speedup
 add_100_edges_to_self              282            283                              1    0.35%   x 1.00
 add_100_nodes                      12,841         12,274                        -567   -4.42%   x 1.05
 add_5_edges_for_each_of_100_nodes  904            899                             -5   -0.55%   x 1.01
 add_adjacent_edges                 24,965         21,053                      -3,912  -15.67%   x 1.19
 add_edges_from_root                25,196         21,096                      -4,100  -16.27%   x 1.19
 bigger_edges_in                    36             37                               1    2.78%   x 0.97
 bigger_edges_out                   35             35                               0    0.00%   x 1.00
 bigger_kosaraju_sccs               5,083          5,049                          -34   -0.67%   x 1.01
 bigger_neighbors_in                36             36                               0    0.00%   x 1.00
 bigger_neighbors_out               35             35                               0    0.00%   x 1.00
 bigger_tarjan_sccs                 2,731          2,794                           63    2.31%   x 0.98
 full_edges_in                      15             15                               0    0.00%   x 1.00
 full_edges_out                     13             13                               0    0.00%   x 1.00
 full_kosaraju_sccs                 991            1,007                           16    1.61%   x 0.98
 full_neighbors_in                  15             15                               0    0.00%   x 1.00
 full_neighbors_out                 13             13                               0    0.00%   x 1.00
 full_tarjan_sccs                   442            441                             -1   -0.23%   x 1.00

@ABorgna ABorgna added this to the 0.6 milestone May 15, 2021
@ABorgna ABorgna added the bug label May 15, 2021
@ABorgna ABorgna merged commit c36e482 into petgraph:master May 16, 2021
teuron pushed a commit to teuron/petgraph that referenced this pull request Oct 9, 2022
* Fix various bugs in matrix graph

- Extension function for directed graphs did not copy the edges
- Off by one in with_capacity
- Check if the lazy adjacency matrix contains node before accesing

* Exact allocation in MatrixGraph::with_capacity

Avoids unnecessary quadratic memory waste.
Dynamic expansions continue rounding to the next power of two.

* Avoid unnecesary bonds checks in update_edge

Speeds up edge inserting benchmarks by ~35%

* Use array swaps when possible when extending

Overlapping slices still use the iterative method.

* Fix bug in iterative copy, add test
teuron pushed a commit to teuron/petgraph that referenced this pull request Oct 9, 2022
* Fix various bugs in matrix graph

- Extension function for directed graphs did not copy the edges
- Off by one in with_capacity
- Check if the lazy adjacency matrix contains node before accesing

* Exact allocation in MatrixGraph::with_capacity

Avoids unnecessary quadratic memory waste.
Dynamic expansions continue rounding to the next power of two.

* Avoid unnecesary bonds checks in update_edge

Speeds up edge inserting benchmarks by ~35%

* Use array swaps when possible when extending

Overlapping slices still use the iterative method.

* Fix bug in iterative copy, add test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge disappearing, or misunderstanding matrix graph?
1 participant