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

Add standard and detailed timers for benchmarking purposes for NEST 2.20.2 #2086

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

jarsi
Copy link
Contributor

@jarsi jarsi commented Jun 30, 2021

This PR adds timer functionality to NEST 2.20.2. The usage and functionality is similar to what is implemented in PR #1778 and #2063 and addresses the issues raised in #1471.

These timers are implemented:

  • time_construction_create (basic timer, always enabled)
  • time_construction_connect (basic timer, always enabled)
  • time_simulate (basic timer, always enabled)
    • nested in time_simulate:
      • time_update
      • time_gather_spike_data
        • nested in time_gather_spike_data:
          • time_collocate_spike_data
          • time_communicate_spike_data
          • time_deliver_spike_data
  • time_communicate_prepare (basic timer, always enabled)
    • nested in time_communicate_prepare:
      • time_gather_target_data
        • nested in time_gather_target_data:
          • time_communicate_target_data

@heplesser heplesser added I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Enhancement New functionality, model or documentation labels Jul 1, 2021
@heplesser heplesser added this to the NEST 2.20.2 milestone Jul 1, 2021
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.

Doing a grep of the code, I still find some GetKernelStatus('time')/GetKernelStatus("time")/GetKernelStatus()['time'] in documentation and examples. Can you update those as well? Other than that, and my comment below, it looks good.

@@ -1511,6 +1521,8 @@ NestModule::Cvgidcollection_iaFunction::execute( SLIInterpreter* i ) const
void
NestModule::Cvgidcollection_ivFunction::execute( SLIInterpreter* i ) const
{
kernel().connection_manager.sw_construction_connect.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

While I can see why it could make sense to include cvgidcollection in the connect time, this has not been added in #2063. Either remove it from this PR or add it to #2063.

@jarsi
Copy link
Contributor Author

jarsi commented Jul 5, 2021

Good catch, thanks for the review. I addressed the issues that you raised.

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, looks good now.

@jougs
Copy link
Contributor

jougs commented Jul 20, 2021

@ackurth: friendly ping!

@ackurth
Copy link
Contributor

ackurth commented Aug 12, 2021

I ran a larger number of benchmarks, everything looked fined.

@hakonsbm hakonsbm merged commit bf1e37d into nest:2.20.2-develop Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants