Atomic gravity and time-step limiter

Changes:

• Use atomic operations for the gravity tasks that update gpart,
• Use locks in the gravity tasks that update multipoles,
• Use atomic operations for the limiter task,
• Do no lock the tree for the gravity and limiter tasks,
• Add an atomic max for chars,
• Minor changes to the parameter files,
• Add an option in the configuration script to revert to the old behaviour.

Implements #359 (closed).

I can add an ifdef to revert to the old behaviour but if you want to already have a look, here it is.

@pdraper your thoughts on this are also more than welcome!

added 2 commits

• 7295cf3d - Only use atomic operations when SWIFT_TASKS_WITHOUT_ATOMICS is not defined
• a984ae3f - Merge branch 'atomic_gravity' of gitlab.cosma.dur.ac.uk:swift/swiftsim into atomic_gravity

Compare with previous version

added 1 commit

• c83f6d0c - Lock the gravity and limiter tasks when SWIFT_TASKS_WITHOUT_ATOMICS is defined

Compare with previous version

added 1 commit

• 74cb8972 - Add an option to disable the use of atomic operations within the tasks and…

Compare with previous version

changed the description

I have now added a configuration option to revert to the old behaviour.

Yes, I thought one of your main concerns was that future implementers would need to be aware of the use of atomics. Having an explicit set of interfaces that provide commit access to the struct seemed to document the don't write directly to the struct. Of course if all these functions are already at the level of never needing modification then this doesn't matter. I was also concerned that the check test sections are not corrected to not use atomics, of course that probably is just a noop in practice.

I see what you mean. That's a good idea indeed. I'll add a layer there.

And yes, the check functions only exist in the atomic flavour. I already feel that maintaining an atomic and non-atomic flavour of the main code will be a pain in the long run. Having to also maintain two versions of the accuracy tests is too much I think.

added 4 commits

• 5ee57ba2 - Hide the atomics into a layer of functions that can be used to update the…
• 85af978e - Apply the same behaviour to the accumulators used to check the accuracy of the gravity calculation.
• 449346aa - Added an atomic max for int
• ac2448b1 - Apply the same change to the max operation taking place in the timestep limiter task

Compare with previous version

What do you think of this updated version?

Certainly looks much cleaner! The only question I have is what happens to inline functions inside inline functions. Not forbidden I expect, just how good compilers are at working that out.

Checking with icc, the assembly generated for the function gravity_cache_write_back() is identical between the current master and this branch with SWIFT_TASKS_WITHOUT_ATOMICS. This tells me all the inlining is dealt with correctly.

Good, so all looks acceptable to me, short of some testing that is.

added 2 commits

• e73aafb3 - Added missing activation of the time-step sync task in black-hole only cells
• 8c8934c5 - Fix the time-step debugging check for the rare case where a BH has swallowed the…

Compare with previous version

I have cherry-picked two fixes that were triggering unrelated debugging checks.

On my side, I am happy with the accuracy of the calculation (unchanged) and the change in speed. Any other test of the code is welcomed though.

resolved all discussions

added 1 commit

• 271b1b15 - Do not report the task times when rebuilding after a repartiton. Do so before…

Compare with previous version

@nnrw56 do you have any feelings about this?