Re: [fluid-dev] Thread safety

[David Henningsson]

address@hidden skrev:
> Hello David,
> 
> Looks like you have done a pretty thorough analysis of FluidSynth
> threading, something which I have not done and still lack some knowledge
> concerning the current code base.  Despite this, I'll try to be
> intelligent in my questions and opinions ;)

I think they are very valid and I appreciate the feedback :-)

> Quoting David Henningsson <address@hidden>:
>> I've been looking at the threading strategy in Fluidsynth, which seems
>> to need some review. While thinking about this, my first priority have
>> been to avoid concurrency issues, at all costs (an underrun is bad, but
>> a segfault is 100 times worse), and my second priority has been to avoid
>> unnecessary thread synchronization costs when it is not needed.
>>
>> Given that priority list, we should make the fluid_synth_t class
>> single-threaded and avoid mutexes altogether. One reason for this is
>> that one of Fluidsynth's most important usages are as a virtual
>> instrument in MusE/Rosegarden/etc, and there I assume it is used
>> single-threaded anyway. And given the current behavior (some amount of
>> synchronization but not fully functional), I think we could say that
>> getting rid of the synchronization would be backwards compatible.
>>
> 
> This would simplify things greatly and eliminate mutex locking stalls in
> the audio thread, which is good.  

Well, the current code uses mutexes only at one place
(fluid_synth_noteon), it has been commented out in all other places. So
there are no stalls for the audio thread at the moment (there are
segfaults instead!).

> My only concern is if there is any
> software which currently uses FluidSynth in a multi-threaded fashion and
> expects it to work, which would require changes in said software, were
> we to make the FluidSynth synthesis core single threaded.  I think this
> number is likely pretty low and I don't think it should prevent us from
> making these changes, but it would be good to let the authors of these
> programs know.  Although these programs might very well be unstable with
> the current FluidSynth.
> 
>> This means some additional work for the audio thread, i e it has to
>> process midi events. I think this is unavoidable for the moment. Btw, if
>> we want to speed up audio processing, I think parallellizing voices (i e
>> two CPU cores could render two different voices simultaneously) could be
>> a fun thing to investigate. But that is in the distant future (at least
>> not scheduled for 1.1.0).
> 
> Processing voices amongst multiple processors would indeed be cool. 
> Perhaps we should keep that in mind as things are redesigned, but save
> it for a future version, as you suggest.  I think there is a lot that
> can be done to optimize and clean up the current synthesis core, which
> would be a good place to start in improving performance.  The last work
> I did on this was to improve the interpolation algorithm (which wasn't
> previously interpolating around loop start/end points).  I was able to
> improve the interpolation and speed things up a little at the same time.
> 
>> Back to the threading. Leaving fluid_synth_t single-threaded means that
>> other threads cannot call fluid_synth_noteon and friends. For the midi
>> player, this is already implemented with the sample timers. For the midi
>> drivers, I'm working on a patch that will use the sequencer as a buffer
>> for this case.
> 
> I wonder what applications currently use these explicitly and outside of
> the audio thread?  I can name one at this point, Swami.  But that is
> easy enough for me to change.  I'm still not seeing exactly how this new
> single threaded architecture would operate though.  Would the sequencer
> act as a queue for events from other threads?  Or would the requirement
> be that the audio thread would process these events itself somehow?

The proposed change is that incoming events will be pushed into the
sequencer queue by the midi thread, then popped and processed by the
audio thread. (The call stack for the audio thread would be:
fluid_synth_one_block -> fluid_sample_timer_process ->
fluid_sequencer_process -> fluid_seq_fluidsynth_callback ->
fluid_synth_noteon and friends)

However, if you say that Swami expects fluid_synth_t to be somewhat
thread-safe (just had a five-minute look a qsynth and at first glance it
seems to expect the same), perhaps it would be bad to enforce the
single-threaded synth at this point. Besides, I like the current idea of
starving the MIDI threads preferred to underrunning the audio in a
heavy-load situation. And that would break.

>> So the thread synchronization is limited to the sequencer. Here's the
>> suggested rules for using the sequencer in a thread-safe way:
>>
>> - Only one thread at a time can call fluid_sequencer_process. This would
>> normally be the audio thread, via the sample timer callback.
>> - Any thread can call fluid_sequencer_send_at. The shortcut from
>> fluid_sequencer_send_at to fluid_sequencer_send_now is removed. This is
>> a subtle change of behavior but it is more consistent with the
>> documentation (i e "MAKES A COPY").
>> - fluid_sequencer_get_tick can be called from any thread (and should be
>> implemented with glib atomic integers).
>> - fluid_sequencer_send_now and fluid_sequencer_process are the only
>> public API calls that can result in midi events being sent. Midi events
>> are sent before the call returns and in the context of the calling
>> thread.
>> - Don't mess around with registering clients, or anything else, while
>> another thread makes a call to the sequencer.
>>
>> A question would be whether this means we should leave up to the
>> libfluidsynth user to insert a sequencer in the midi processing chain,
>> or if we should create it automatically behind the scenes somehow (and
>> if so, what component should create it?). What do you think?
> 
> I don't think I fully understand this last bit.  I guess all events
> would go through the sequencer?  If that were the case, I could see how
> the fluid_synth_t would create it.  Maybe even the fluid_synth_noteon
> and friends could go through this?

Right. But if we were to go that way, perhaps we need all calls to go
through the sequencer, not only the ones that translate directly to a
midi event. Which probably means additional work.

My patch - in its current state - does not touch the synth. It changes
the sequencer according to the text above, and fluidsynth.c manually
inserts a sequencer between the midi router and the synth. So
libfluidsynth applications such as Swami and qsynth will neither suffer
or gain from this patch, unless they do the same.
What do you say if we leave it at that for the moment, I commit the
patch and we can all test it to see if we find any difference in latency
or stability when we use fluidsynth from the command line?

And at a later point in time we could review the synth threading a bit
more deep, to see if we can improve the situation (with regards to
segfaults, parallellization, stalls etc)?

// David