Disentangle kernel-manager include tree

[JanVogelsang]

Summary of changes

This PR fixes the horrible kernel-manager singleton design and cleans up all circular include chains which were just hidden behind the kernel-manager and plenty "*_impl.h" files.
Even though this PR touches 250+ files, the only functional changes were done in the kernel_manager.h/.cpp and the other changes are just follow-up refactorings enabled by the changes. I also got rid of most of the impl-files, which are not required anymore and just complicated things.

Why this was necessary

Anyone who ever worked on the kernel will immediately know about the pain the kernel-manager caused. As the kernel-manager included ALL other manager in the kernel_manager.h file, any other class using the kernel-manager therefore also included ALL other managers. This resulted in either of two things:

1. Any class using the kernel manager could not be used by ANY other manager anymore, as this would lead to a circular include chain.
2. Any class used by other managers was not allowed to include the kernel-manager in its .h file. This meant:
   2.1. The kernel-manager could only be included in the .cpp file, which meant that the function using the kernel-manager (and thus any of the other managers) could not be inlined, which in some cases is detrimental for performance.
   2.2. We had to create those impl.h files and use the kernel-manager in there. However, impl.h files and correctly including them is a nightmare and leads to various linker issues which are extremely difficult to debug.

The only time when one should use impl.h files is for some template specializations.

[JanVogelsang]

Important to note: I kept the internal API identical. All I've done essentially was to use references to the managers instead which allowed to forward-declare the managers in the kernel_manager.h file. The reason there are that many changes is that including the kernel_manager.h no longer means that one also includes ALL other managers. So each file using a specific manager has to include that manager explicitly. This is much cleaner and will also speed up compilation a bit.

[JanVogelsang]

I have no idea what's supposed to be wrong with the 2 copyright headers. @terhorstd Could you take a look please?

[heplesser]

@JanVogelsang This looks very interesting! Just to make sure I understand your solution correctly: Instead of having the individual managers as actual data members of the KernelManager instance, the KernelManager now holds references to the different managers, so that kernel_manager.h only needs forward declarations of the various *Manager classes, right? That then allows removing all the inclusions and the impl-files.

This is certainly a great code cleanup. I am just a tiny bit concerned about performance, since we now have essentially pointers to the individual managers. Have you run any benchmarks yet?

[heplesser]

I have no idea what's supposed to be wrong with the 2 copyright headers. @terhorstd Could you take a look please?

Very strange. Have you tried to run the copyright check locally on your machine?

[JanVogelsang]

@heplesser Yes, this is correct!

I benchmarked the performance and the PR's solution is indeed around 5% slower (locally and on Juwels). I have to admit that this surprises me a lot, as while this introduces another pointer, I would have assumed that all these pointers (or mostly just the simulation_manager and event_delivery_manager pointers during simulation) would be cached anyway. This seems to not be possible and I am not sure why. Then again, the cause of the performance degradation might also be something entirely different, for example growing code size which harms instruction cache efficiency due to more function inlining.

I implemented two other solutions now, which both require the internal API to change, though:
kernel().simulation_manager ->kernel::manager<SimulationManager>()

1. A template function with a function-local static manager instance.

namespace kernel {
template<typename ManagerT, std::enable_if_t<std::is_base_of_v<ManagerInterface, ManagerT>>* = nullptr>
ManagerT& manager()
{
  static ManagerT static_manager;
  return static_manager;
}
}

2. An inline (global) variable for each manager.

namespace kernel {
template <class T>
inline T g_manager_instance;

template <class T>
T& manager() noexcept {
  return g_manager_instance<T>;
}
}

I'm currently benchmarking both solutions, but local benchmarks suggest that the second approach performs best.

[JanVogelsang]

I have no idea what's supposed to be wrong with the 2 copyright headers. @terhorstd Could you take a look please?

Very strange. Have you tried to run the copyright check locally on your machine?

I managed to solve this, it was just two empty files which I deleted but somehow they were added back in again, just empty.

[heplesser]

@JanVogelsang Thanks for checking and the new solutions! I slightly prefer the second option, but that is mainly based on the fact that I still haven't entirely wrapped my mind around the std::enable_if_t construct.

Is it necessary to place this into a separate namespace kernel? Why not have it in the nest namespace and then just use it as manager<SimulationManager>() or maybe even get<SimulationManager>() or (since we have get in many places) use<SimulationManager>() or something like that?

[terhorstd]

Did these lines move the wrong way around? layer.h was deleted, but the _impl file now has all code?
What is the reason for dropping the include-guards?

[JanVogelsang]

Thanks for catching this, it definitely wasn't intentional. I think I first removed the layer_impl.h, but then added it back again as there was a circular dependency with the grid_layer.h. So I must have accidentally copied too much from the layer.h over to the layer_impl.h

[terhorstd]

@jessica-mitchell, this would be a perfect place to link into the Doxygen documentation instead of copy-pasting some random code file into the RST.
For now however some reference syntax could shorten this somewhat unrelated change in this PR. Isn't there a possibility to link to a code file to be included? Instead of this PR updating the docs with the new file, it would be preferable to have something like:

.. code: cpp
   file: ../nestkernel/stopwatch.h

Would avoid this excessive change in an rst file.

[terhorstd]

@JanVogelsang, this is big work! Thanks a lot for this big cleaning effort!
Currently there still seem to be some build errors, but most of the changes seem fine to me. I commented a few places already, and will do a proper review once the CI passes.

Could you also check the relation to #3465 ?

[JanVogelsang]

@JanVogelsang Thanks for checking and the new solutions! I slightly prefer the second option, but that is mainly based on the fact that I still haven't entirely wrapped my mind around the std::enable_if_t construct.

Is it necessary to place this into a separate namespace kernel? Why not have it in the nest namespace and then just use it as manager<SimulationManager>() or maybe even get<SimulationManager>() or (since we have get in many places) use<SimulationManager>() or something like that?

That's totally fine as well. The additional/nested namespace (nest::kernel::) was just intended to make the transition for other devs easier, as kernel().X_manager isn't that far away from kernel::manager<XManager>(). But of course, any other flavor of that is great as well. I think I like the use<>() flavor most, but that's just personal taste.

[JanVogelsang]

Benchmark results from JUWELS. HPC Benchmark weak scaling with 2 processes per node, 24 threads per process, scaling the number of nodes ("1,2,4,8,16,32") and a scale of 8 per node (i.e., 45k Neurons per process, 90k per Node). The y-axis shows the C++ simulate timer (not including presimulation) for a simulation time of 1s. All times were averaged over 3 runs, each run with a different seed (but the same 3 seeds for each parameter combination).

While there is quite some fluctuation, it seems like the inline-approach (number 2 above) performs best and on par with master.

[JanVogelsang]

@JanVogelsang, this is big work! Thanks a lot for this big cleaning effort! Currently there still seem to be some build errors, but most of the changes seem fine to me. I commented a few places already, and will do a proper review once the CI passes.

Could you also check the relation to #3465 ?

It's still compatible with that PR, the role of the kernel manager stays the same.