Performance suggestions about molly.jl

[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]

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]

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

• 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]

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.