Fluid Particles: fix viscoelastic spring threading crash again after D7394.
ClosedPublic
Actions

Authored by Alexander Gavrilov (angavrilov) on Nov 23 2020, 4:28 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Bastien Montagne (mont29)
Jeroen Bakker (jbakker)

Commits

rB02a0f6b04a64: Fluid Particles: fix viscoelastic spring threading crash again after D7394.

Summary

Since D6133 fluid particle code uses thread local storage to collect
springs created during a time step before adding them to the actual
spring array.

Prior to the switch to TBB there was a single finalize callback which
was called on the main thread, so it could use psys_sph_flush_springs
and insert the new entries into the final buffer. However in D7394 it
was replaced with a reduce callback, which is supposed to be thread
safe and have no side effects. This means that the only thing it can
safely do is copy entries to the other temporary buffer.

Diff Detail

Repository

rB Blender

Branch

temp-angavrilov-fix-fluid (branched from master)

Build Status

Buildable 11412
Build 11412: arc lint + arc unit

Event Timeline

Alexander Gavrilov (angavrilov) requested review of this revision.Nov 23 2020, 4:28 PM

Alexander Gavrilov (angavrilov) created this revision.

Harbormaster completed remote builds in B11397: Diff 31306.Nov 23 2020, 4:28 PM

Bastien Montagne (mont29) added inline comments.Nov 23 2020, 5:08 PM

source/blender/blenkernel/intern/particle_system.c
3702	This only gets called once now at the end of the process (as opposed to after each step in the classical solver case), and not at all even in the DDR solver case. Not sure whether this is important or not (did not follow enough code to check), but either way this change should be clearly analyzed and explained in commit message imho?

Alexander Gavrilov (angavrilov) added inline comments.Nov 23 2020, 5:25 PM

source/blender/blenkernel/intern/particle_system.c
3702	How is it 'not at all' when it's called `from psys_sph_finalize` that cleans up the SPHData structure? Also, it seems that springs are only added by the DDR solver anyway, because the only place they can be added is `sph_force_cb`, which is only referenced from `psys_sph_init` for the `SPH_SOLVER_DDR` case. The original fix was probably being paranoid. Should I remove the extra references to reduce while I'm at it?

Removed unneeded use of reduce for the classical solver, as rechecking the code reveals it doesn't support the option that dynamically adds springs.

Harbormaster completed remote builds in B11412: Diff 31340.Nov 24 2020, 7:26 PM

Bastien Montagne (mont29) accepted this revision.Nov 25 2020, 12:55 PM

Bastien Montagne (mont29) added inline comments.

source/blender/blenkernel/intern/particle_system.c
3702	How is it 'not at all' when it's called from `psys_sph_finalize` that cleans up the `SPHData` structure? Ugh you are right, sorry, I missed one level of indentation here (that's shy I hate that two-spaces indentation...).
source/blender/blenlib/intern/buffer.c
117	Comments are supposed to be proper sentences, with capital, final point, etc.