Improve speed of dump_connections() in layer_impl.h

[xavierotazuGDS]

Instead of calling get_connections() inside a loop for every source node, it calls get_connections() once for all connections between layers.

The code considers that:

1. source nodes positions (src_vec) is ordered by source node.
2. connectome is ordered by source node.

fixes #3142

[xavierotazuGDS]

This is a proposed solution for issue #3142

[xavierotazuGDS]

This implementation only works for non-MPI and in some cases (not all) of MPI. Hence, DO NOT USE !!!!

[heplesser]

@xavierotazuGDS Thank you for your PR! Since there still seems to be a problem with the code as of your last message, I have converted it to "Draft" status. You can change that back again later.

[xavierotazuGDS]

I have uploaded a new version. No bugs now on MPI. I do not change the Draft status because I am not sure how to do it.

[xavierotazuGDS]

Status changed from "Draft" to "Open".

[terhorstd]

Thanks for the update, xavierotazuGDS!

There are/were still some formatting issues (mostly regarding white-spaces as far as I see), but they need to be fixed to keep code in a consistently formatted state (see Contribute to NEST).

Since you seem to be a first time contributor, please also send a Contributor Agreement to [email protected] so we can accept and correctly acknowledge your contribution.

[terhorstd]

I just saw, that you already did send the CA! My appologies.

[xavierotazuGDS]

terhorstd,

I believe I solved all my code style issues. The other issues are in methods I have not modified (they are clang-format issues from the original code). Anyway, I could try to solve them (I will need some time).

[xavierotazuGDS]

I took a look at clang-issues in methods I have not modified, but suggested changes are so big code blocks that I would not know what I am doing. Hence, I prefer not to modify clang-format issues outside the method I modifed.

[terhorstd]

The version of the code formatter changed some time back, this is why there are complaints. Everything is already fixed in the main branch, which your branch is several commits behind (see on the main page of your repo "… behind"). Try to merge those into your branch, that should fix all spots that you didn't touch.

[otcathatsya]

I ran some basic benchmarks on the JURECA cluster with the following setup:

• 1 node, 64 threads
• Scaling number of layers (100x100 grid each) connected to every other layer (and itself) with a 25x25 mask
• Average over 10 runs, call DumpLayerConnections after each layer connection

Layers	Num Total Conns	Time	Final Memory	Branch
1	6.250.000	19.76s	2873MB	Improved
2	25.000.000	81.15s	3891MB	Improved
4	100.000.000	322.11s	7743MB	Improved
1	6.250.000	28.22s	1600MB	Master
2	25.000.000	139.72s	2735MB	Master
4	100.000.000	1070.84s	6330MB	Master

This seems to indicate that it scales pretty linearly, not even increasing memory usage by much

[github-actions]

Pull request automatically marked stale!

[heplesser]

@xavierotazuGDS My apologies for taking a long time to react to your PR. Your code is largely fine, but can be come quite a bit cleaner and simpler by using C++'s find_if() function. I have created PR against your repo that implements it. Once you accept that PR in your repo, it will automatically pop up here and I will approve.

We would like to integrate this improvement into the release candidate for NEST 3.8 within a few days.

[heplesser]

@otcathatsya I have worked a bit more on the improved dump_connection(). You can find my current status here: https://github.com/heplesser/nest-simulator/tree/xdump. Could you run a subset of your benchmarks again on that branch to ensure I did not introduce performance regressions?

[heplesser]

@otcathatsya I have worked a bit more on the improved dump_connection(). You can find my current status here: https://github.com/heplesser/nest-simulator/tree/xdump. Could you run a subset of your benchmarks again on that branch to ensure I did not introduce performance regressions?

My branch is now merged here, so you can work on @xavierotazuGDS's branch for this PR.

[heplesser]

@xavierotazuGDS The merge worked and this looks good now, so I approve. Before merging, we need to wait for benchmark data by @otcathatsya.

[otcathatsya]

Sorry for the hold-up, here are the benchmarks for the latest commit:

[heplesser]

@otcathatsya Thank you for the data—and good they look, so here we merge ...