Performance suggestions

[SebastianM-C]

I've looked at the performance of a standard liquid argon simulation and I'd like to propose some small design changes.

• [x] The interactions could be stored in tuples or named tuples if key based indexing is required. This should be coupled with adding another type parameter (for each) in the structure definition. The idea of this change is to avoid the presence of abstract types in the acceleration loop. With the current design type inference is not able to obtain concrete types and thus there are some allocations that could be avoided.
• [x] The force! functions should return nothing since with the current design they are type unstable as the if branches at the beginning can return nothing. Since these are in-place functions, returning the forces does not seem particularly useful (and it is not used as far as I can tell). Due to the small union optimizations this is not a big performance problem, but it would be nice to improve the situation.

If you agree, I can make PRs to address this changes.

[jgreener64]

1. Sounds worth a go. I think this could be possible without changing the high-level API. Not that I am against changing the API, just that any performance gains that complicate the API might not be worth it.

2. I guess I was going for the "It is typical for such functions to also return the modified array for convenience" guidance at https://docs.julialang.org/en/v1/manual/style-guide/#bang-convention-1, but this is loose guidance and I forgot to change the other shortcut returns. It is probably more intuitive to return nothing here as you suggest.

I just gave you write access to the repo so feel free to work on a branch, obviously PR things for review before merging to master though.

[SebastianM-C]

Some more ideas would be to cache neighbors and accelerations. This could potentially have a bigger impact than the above.

• [x] Store neighbors in the Simulation object. This would could help reducing allocations by using a pre-allocated cache for neighbors in the simulation object.
• [ ] Store a cache for accelerations in the Simulation object

[jgreener64]

Yes, sounds worth a go.

I used to have the neighbours as part of the Simulation object, I might not have been doing it in the right way to re-use memory though.

[SebastianM-C]

@jgreener64 Regarding neighbors is it safe to assume that all NeighbourFinder return a Tuple{Int, Int}[]? I was not sure if the cache should be the same for all NeighbourFinders (and in the respective structs) or in the Simulation object directly and the same for everyone.

[jgreener64]

Tuple{Int, Int}[] was what I went for as a simple scheme for recording pairs, though I've only implemented the distance neighbour finder so far so I don't know what works best in general.

For now I think we can assume that all NeighbourFinders return a Tuple{Int, Int}[] if that's helpful to you, though I don't have very strong feelings on it and am open to better schemes. Currently the requirements seem to be that you can index into it to get something containing i and j. A variety of structures would work with this so one option is to have it as another type parameter, depending on whether this allows you to cache effectively.

[SebastianM-C]

In this case I'll just go with Tuple{Int, Int}[] for everything and if we need more we can change it in the future.

[SebastianM-C]

I noticed that even when I optimize a single acceleration function call to be faster than the one in NBodySimulator, the full simulation is still 2x slower. I think that adapting for the DiffEq time-stepping could give significant performance gains. The main idea would be to replace simulate! with an appropriate solve call. This would mean that things such as finding neighbors and thermostats will be implemented as DiscreteCallbacks. I think that the construction of the DiffEq problem can also be handled in ParticleAccelerations.

[jgreener64]

Yeah that would be good.