Skip to content

Conversation

JanVogelsang
Copy link
Contributor

This PR is a simplified version of #3581 which doesn't require C++20.
It simplifies the stopwatch implementation by combining all template specializations into a single one which uses if constexpr constructs and some clever std::conditional_t usage to use the correct timer containers and functionality based on compile flags.

@JanVogelsang JanVogelsang self-assigned this Sep 19, 2025
@JanVogelsang JanVogelsang added S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation labels Sep 19, 2025
Fix problems and tidy up code
@heplesser
Copy link
Contributor

@JanVogelsang Concerning the placement of the assert_thread_parallel() test in

https://github.com/JanVogelsang/nest-simulator/blob/0373a93b8cc88d8d46843e04f59fb428eb38eeae/nestkernel/stopwatch_impl.h#L39-L46

and corresponding locations, consider what happens when we add nodes to the network in

https://github.com/JanVogelsang/nest-simulator/blob/0373a93b8cc88d8d46843e04f59fb428eb38eeae/nestkernel/node_manager.cpp#L104-L106

Here, sw_construction_create_ is a master-only timer and it is started in a serial context. Indeed, because it is a master-only timer, it must only be started/stopped from a serial context. For this timer, because it is master-only, we will have enable_timer == true (always, because it is a normal granularity timer) and use_timer_array == False (always, because it is a master-only timer). Now if we had assert_thread_parallel() inside the enable_timer but outside the use_timer_array conditions, the assertion will also be tested for master-only timers correctly called from serial context and then it must fail.

So the assertion must only be tested when we actually have a threaded timer, i.e., inside the use_timer_array. Then it needs to be tested, because if we started/stopped threaded timers in a serial context we would end up with inconsistent results.

No, strictly speaking, we could argue that timers started/stopped from parallel contexts should always use a timer array. The with-threaded-timers option should not exist. Then, instead of the #pragma omp master we could have a assert_single_threaded(). But since we want to support single-valued timers also in parallel contexts for backward compatibility, we need the pragma and cannot have the assertion. I will suggest a comment.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@JanVogelsang Very nice work! I just added one suggestion for a comment.

@heplesser
Copy link
Contributor

BTW, I have checked all four detailed/threaded combinations and they work as intended. Very nice that now only the actually active timers show in the kernel dictionary.

Co-authored-by: Hans Ekkehard Plesser <[email protected]>
@JanVogelsang
Copy link
Contributor Author

I see, so essentially we have two different non-threaded timer types:

  1. True "master-only" timers which are called in a parallel environment, but only by the master thread.
  2. Timers in a serial environment.
    In theory we could distinguish these two types as those are different cases, but as the #pragma omp master works even in serial environments, it's fine to just use the same code for both cases.

@heplesser
Copy link
Contributor

heplesser commented Sep 20, 2025

I see, so essentially we have two different non-threaded timer types:

  1. True "master-only" timers which are called in a parallel environment, but only by the master thread.
  2. Timers in a serial environment.
    In theory we could distinguish these two types as those are different cases, but as the #pragma omp master works even in serial environments, it's fine to just use the same code for both cases.

I would state it the other way around. I would consider timers such as

  Stopwatch< StopwatchGranularity::Normal, 
             StopwatchParallelism::MasterOnly > sw_construction_create_;

which are explicitly defined as MasterOnly as "true master-only" timers. These should only ever be called from a serial context.

Then we have timers declared as thread-parallel, e.g.,

Stopwatch< StopwatchGranularity::Normal, 
           StopwatchParallelism::Threaded > sw_communicate_prepare_;

which we coerce to run master-only when Dwith-threaded-timers=OFF is set. This does not really make sense, we only do it for backward compatibility with the way we took measurements in earlier versions of NEST.

@jessica-mitchell jessica-mitchell requested review from gtrensch and removed request for terhorstd September 22, 2025 06:52
Copy link
Contributor

@mlober mlober left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this so quickly. All my tests with the various configuration options were successful.

Copy link
Contributor

@gtrensch gtrensch left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. 👍

@gtrensch gtrensch merged commit ba397c5 into nest:master Sep 22, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants