Page MenuHome

Fluid Particles: fix threading crash with viscoelastic springs.
ClosedPublic

Authored by Alexander Gavrilov (angavrilov) on Sat, Oct 26, 10:07 AM.

Details

Summary

As correctly pointed out by a comment in the code, adding
new springs wasn't thread safe, and caused crashes. Fix by
buffering new springs in intermediate thread-local arrays,
which are flushed on the main thread.

Diff Detail

Repository
rB Blender

Event Timeline

I'm not sure if this solution is really threadsafe.
Looks like sphdata->new_springs can be written and read at the same time.

This should also have a performance penalty as it is necessary to allocate a different buffer and flush when possible.
I still think it would be good to link this patch to some report demonstrating the crash.
And profiler to see the impact on peformance.

@Sergey Sharybin (sergey) and @Bastien Montagne (mont29), as you have worked with the blender thread API a few times, any comments on this patch?
The idea is to make sph_spring_add thread safe.

Looks like sphdata->new_springs can be written and read at the same time.

That is thread local storage, there is a separate copy for each worker thread.

And profiler to see the impact on peformance.

The impact on performance is immaterial compared to a crash.

There is one place where new items are added, but the old items are constantly read by all threads - that's why it crashes so readily. So postponing the add to be flushed on the main thread is the only option. Flushing could probably be done with one memcpy instead of adding the items one by one though.

Postponing is possible because the items are accessed via an index lookup hash that is rebuilt once per physics step, so the new items are invisible until the next step starts.

Patch LGTM, and seems like the best approach, considering newly added springs during a step are not used until next step anyway.

Note that some extra mem allocation & copying will have a neglectable effect on such heavy-demanding simulation. And almost certainly much less impact than adding some thread sync (mutex or so) to make call to sph_spring_add thread safe especially on heavy workstations with 64 threads or more.

I guess @Campbell Barton (campbellbarton) might want to double check naming/consistency of the changes to BLI_buffer?

This revision is now accepted and ready to land.Thu, Nov 7, 5:39 PM