Use vector of integer for per-thread bool indicators

[suku248]

• Resolve Spike loss in repeated simulations of exact same script #1394
• Rename class CompletedChecker to class PerThreadBoolIndicator and adapt all instances accordingly
• Support subscript read/write access to elements of PerThreadBoolIndicator
• Use std::vector of BoolIndicatorUInt64 instead of array of bool in PerThreadBoolIndicator
  (BoolIndicatorUInt64 is a wrapper for uint_fast64_t status_ that restricts functionality to is_true/false(), set_true/false(), and logical_and())
• SourceTable members is_cleared_ and saved_entry_point_ are now of type PerThreadBoolIndicator instead of std::vector<bool >

[heplesser]

@suku248 Thank you very much, this is very nice! I particularly like your idea to introduce the BoolIndicatorUint64 type to restrict operations. I have some minor questions and suggestions in the code.

Since the issue addressed by this PR is not detected by the test suite and a normal regression test is not feasible, could you add some information to the PR about how you have checked that this PR solves #1394?

[jakobj]

Thanks @suku248 for this awesome implementation! :) looks already good and makes also other code easier to read (e.g., in source_table.h). as @heplesser i was also wondering about the omp barriers, maybe you could clarify that. just a few additional comments.

[jakobj]

doesn't this set status_ to and actual bool instead of using the true_uint64/false_uint64 values?

[suku248]

The right hand side results in a bool, which is then converted to either integer 0 or 1 when assigned to status_. This is consistent with the initial assignment of true and false to the constants true_uint64 and false_uint64, respectively. We could use an if/else here to assign true_uint64/false_uint64 but this might be a bit over the top.

[jakobj]

why declare friend here? aren't you only using public functions in PerThreadBoolIndicator?

[suku248]

Good point! Thanks! This was required in an earlier version of the code. I have removed this now.

[jakobj]

maybe also provide an operator[] that returns const references in case one is only interested in reading the value?

[suku248]

I would say: To be added as the need arises.

[jakobj]

if they'd be provided, wouldn't such calls as in

nest-simulator/nestkernel/connection_manager.cpp

Line 558 in 4b15e1a

if ( check_primary_connections_[ tid ].is_false() and is_primary )

use the const version? depends on what you mean by "need" ;)

[suku248]

I think check_primary_connections_ would have to be const in order to result in a call to the additional constant operator[] function you are suggesting. But please feel free to try this out and, of course, adapt the code in case you are right. I just don't want to add a function that is currently not used.

[suku248]

@jakobj @heplesser Thank you for your suggestions! I think I have addressed all of your comments. Please take another look.

I have repeatedly run the hpc_benchmark (adapted to use only a single synapses type for all outgoing connections of excitatory neurons, see #1394) using 32 MPI processes and 24 threads per process (at least 40 repetitions). With the code changes introduced in this PR I no longer observe any variations in spike counts across different runs.

[jakobj]

thanks for the update @suku248. two more "soft" comments, but i'd also be fine with merging as is. can't believe this is finally fixed 🎉

[stinebuu]

I haven't looked at this, but I am pressing approved so there is no confusion over the comment part of the review. I am very happy it is fixed, good job! 😄