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

Remove set sender node id from deliver events pr #2280

Merged

Conversation

suku248
Copy link
Contributor

@suku248 suku248 commented Jan 27, 2022

Retrieving the node id from SourceTable is computationally costly, and most targets don't need the node id (with the exception of binary_neurons). To support binary_neurons, an alternative solution is suggested here: The location (tid, syn_id, lcid) of the sender node id in SourceTable is stored in Event instead of the node id retrieved from SourceTable, and Event now provides a function to perform the look-up of the node id in SourceTable.

For local receivers (i.e., recording devices), delivery anyway takes a different, more direct path:

kernel().connection_manager.send_to_devices( tid, source_node_id, e );

The node id is retrieved from the local sender and stored in Event, and it can subsequently be retrieved through Event::get_sender_node_id().

@suku248 suku248 requested a review from jakobj January 27, 2022 13:52
@suku248 suku248 self-assigned this Jan 27, 2022
@suku248 suku248 requested a review from hakonsbm January 27, 2022 13:52
@suku248 suku248 added this to PRs in progress in Kernel via automation Jan 27, 2022
@stinebuu stinebuu added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Jan 28, 2022
Copy link
Contributor

@hakonsbm hakonsbm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, I have just a small suggestion.

nestkernel/event.cpp Outdated Show resolved Hide resolved
Kernel automation moved this from PRs in progress to PRs pending approval Feb 4, 2022
Copy link
Contributor

@jakobj jakobj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks for implementing this! 🚀
i have just one small comment, regarding the use of = for SpikeData, but maybe i was blind.

another thing i'd like to mention: as far as i understand, in binary neurons the gid is just used distinguish different presynaptic partners in the receiver node. wouldn't it be possible to instead use the spike data content to do that without looking up the gid? this would reduce (unpredictable?) memory access, but likely require more computation as the indices would need to be somehow combined into a unique identifier for each presynaptic neuron.

@@ -63,6 +63,8 @@ class SpikeData
SpikeData( const SpikeData& rhs );
SpikeData( const thread tid, const synindex syn_id, const index lcid, const unsigned int lag );

SpikeData& operator=( const SpikeData& rhs );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i couldn't see where this was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to silence a compiler warning through GitHub Actions. If I remember correctly, the problem was that since C++11 if a custom copy constructor or destructor is defined, a custom operator= also needs to be provided or it should be explicitly defined as default.

@jarsi
Copy link
Contributor

jarsi commented Feb 21, 2022

Thank you @suku248 very much for this PR. I ran some benchmarks of this PR and the corresponding master on JURECA DC using a static version of the hpc benchmark. This PR is faster than master on all tested scales (1 to 64 nodes, 4 MPI processes per node, 32 threads, 250.000 neurons per Node). The performance difference is around 20%.

Co-authored-by: Håkon Bakke Mørk <hakon.mork@nmbu.no>
@suku248
Copy link
Contributor Author

suku248 commented Mar 10, 2022

Thanks @jarsi for running some performance tests for this!
And thanks @hakonsbm and @jakobj for the review!

@jakobj the solution for binary neurons that you suggest sounds promising. Could you document it in an issue?

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Kernel automation moved this from PRs pending approval to PRs approved Mar 10, 2022
@hakonsbm hakonsbm merged commit cd4b0b4 into nest:master Mar 10, 2022
Kernel automation moved this from PRs approved to Done (PRs and issues) Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants