AudioThreadGuard - keep your audio thread clean


#1

Hi everybody,

I have been toying around with a class that tries to prevent stupid calls on the audio thread (eg. String creations or HeapBlock allocations) and I think this might be a good addition to JUCE.

In production code, you most likely won’t need it, however I am running a scripting engine in the audio thread and people are starting to do stupid things there, so this prevents the scripting code from doing allocations, etc.

I also have tracked down a few allocations that are not super critical, but might cause issues under certain conditions (ScopedReadLock::tryEnter, I am looking at you).

How to use it

1. Enable the JUCE_ENABLE_AUDIO_GUARD macro

If this is disabled, all calls to AudioThreadGuards get redirected to a dummy object so the compiler will most likely throw it away. This results in effectively no overhead if not wanted.

2. Subclass AudioThreadGuard::Instance

You can define a custom logic for testing / logging with this class

3. Create a AudioThreadGuard in your processBlock method

void processBlock(AudioSampleBuffer& b, MidiBuffer& m)
{
    AudioThreadGuard guard(pointerToMyGuard);

    // From now on all calls to illegal methods will fire a warning until guard goes out of scope.
};

4. Add the warning code to illegal operations

In every method that should not be called from the audio thread, add this line:

WARN_IF_AUDIO_THREAD(condition, operationId);

The condition argument can be used to prevent false positives as most operations do no harm under certain circumstances (eg. setting an array element without reallocating).

The operationId is an integer that can be used to identify the culprit if debug symbols are not present (otherwise calling SystemStats::getStackBacktrace() in your warning code is the best option during development).

For example, the destructor of the HeapBlock will look like this:

`   ~HeapBlock()
    {
		WARN_IF_AUDIO_THREAD(data != nullptr, IllegalAudioThreadOps::HeapBlockDeallocation);
        std::free (data);
    }

@Jules is this a sensible addition? I know this would mean adding the warnings at a lot of places in the JUCE codebase. The WARN_IF_AUDIO_THREAD macro will get ifdefed away if not used but it would add some verbosity to these methods (however I don’t think that’s such a bad thing since it explicitly states that the method has nothing to do in an audio thread).

How it works internally

When an AudioThreadGuard is created, it will fetch the current thread id and add it to the list of audio thread ids (this should make it work in multithreaded audio applications). Whenever you call WARN_IF_AUDIO_THREAD, it will check if the current thread id is in the list of audio threads and fire the warning after passing a few tests.

There are a few helper classes that suspend the guard (useful when printing debugging strings), or temporarily set another Instance.

Below you can find the code (same license as all juce_core files).

I tried to follow the JUCE coding style as much as possible making a possible merge into the official codebase more straight-forward - although I am sure you nit-picky guys find something :slight_smile:

juce_AudioThreadGuard.cpp (3.4 KB)

juce_AudioThreadGuard.h (6.4 KB)


#2

Yeah, we’ve discussed doing a similar thing but never quite got around to trying it. Definitely something we’ll consider!


#3

The implementation is pretty simple, although I am not sure if it will work 100% correctly in a multithreaded environment - it has to use a static object for checking (similar to the Logger class), so there might be a few situations where this could go wrong.

I have added the calls to these methods so far:

  • String::createUninitialisedBytes() : catches all String allocations
  • all HeapBlock methods: catches most allocations of JUCE classes, however the default malloc calls and raw new / delete still slip through and overriding the global allocator is a bit rude for this purpose.
  • MessageManagerLock: not sure why you would ever want this in the audio thread.
  • AsyncUpdater::triggerAsyncUpdate: as stated in the comments, this allocates a message, so you shouldn’t use it in the audio thread

Are there any other places on top of your head that are obvious no-go areas for the audio thread?

Btw: the guard fires at AudioSampleBuffer::setDataToReferTo(float** otherData, int numChannels, int size) because it frees the internal channel data array even if the buffer was created with AudioSampleBuffer(0, 0);. If the Buffer is created with the default constructor, it doesn’t complain, however I would think that if the size and channels are zero, it won’t allocate in the first place.


#4

Over the last few years while contributing to Ardour, I developed a logging and instrumentation library to do this sort of thing in a generic way. The library is open source (GPLv3) and can be found here. I have implemented Gtk and Qt GUI frontends for visualisation and control of the library but you don’t strictly need to use a GUI to use the library (It is just much more useful IMO).

It has been on my TODO list to look into adding a JUCE frontend but I’m not sure I’ll get around to it. The library was intended to be able to be used standalone but something like it could be added to JUCE.

It is hard to describe briefly what it can do. If your considering adding something like this to JUCE it might be worth watching a presentation I made about it on Vimeo, perhaps the most relevant part of it is this part on allocation.


#5

This sounds useful. What I think would also be useful, is if somebody could put together a more comprehensive list of calls to avoid making on the audio thread. I mean, I know its possible to learn this the hard way through profiling. But a thread with a few tips would be great!


#6

You will have a hard time tracking down performance issues that come from malloc and friends using a profiler, since it has almost no effect on general throughput / average performance (what a profiler usually measures), but a non-deterministic worst-case behaviour, which can result in clicks even if it runs fine on your system.

But here we go:

  • String creation: Big Nope
  • ValueTree operations on nonexistent data: Big Nope
  • Creation / Destruction of DynamicObjects and setting nonexistent properties: Nope
  • ValueTree operations on existent data: OK as long as you are aware of possible multithreading issues and you are certain that there are no UndoManager and listeners attached (because they would get called synchronously). But better use a CachedValue<> for accessing ValueTree data from the audio thread.
  • Undoable operation: Big Nope
  • Anything that resizes a HeapBlock: Big Nope
  • AsyncUpdater calls: Small Nope (allocates a post message, which is usually very small but nonetheless). The usually suggested solution is to use a Timer with a flag, however if you have a lot of objects that might trigger an update (> 500), the Timer thread will get very busy.
  • new / delete, malloc, free: Huge Nopes
  • alloca: this allocates on the stack so it’s OK. I use this once in a while for scratch buffers where I know the size doesn’t exceed 500-1000 float numbers.
  • MessageManagerLock: Nope Level 9000
  • File I/O: Big Nope
  • UI operations (any method of the Component class: Big Nope

to be continued…


#7

Do you have any thoughts or experience regarding the following on the audio thread:

  • Mutex
  • Atomic
  • Vector
  • WeakReference

#8
  • Mutexes are OK, however you need to be very careful what you do while locking on other threads because you can get priority inversions. Also, the same rules that apply to the audio thread apply to locked code on other threads. The JUCE synthesiser class eg. uses a CriticalSection for all note events so you can start notes from the UI thread.
  • Atomics are fine - actually it’s the recommended way to interact with other threads.
  • std::vector can be fine, however you need to make sure they don’t reallocate during audio processing (same as juce::Array)
  • WeakReferences and ReferenceCountedObjectPtrs are fine (they use atomic reference counting so it’s thread safe). However if you’re in a tight loop, the overhead vs. a raw pointer is noticeable, so this:
for(i = 0; i < numSamples; i++)
{
   weakReference.get()->compute(data[i]);
}

is not very elegant.


#9

I’m not sure there can be a “comprehensive” list as this could always grow and change.

Isn’t what you’re really trying to track here simply two things:

  1. Memory allocations using the global head (custom allocators may be ok)
  2. Things that take locks (possibly due to a system call but more importantly priority inversion on the message thread)

I think these are really two separate issues though and could possibly be handled differently.

I’ve been toying with an idea that would address issue 1 but not had a chance to implement anything yet. I’ll see if I can carve out some time to see if it works and add it to pluginval if it does.


N.B. surely you can’t do this between threads:

for(i = 0; i < numSamples; i++)
{
   weakReference.get()->compute(data[i]);
}

What happens if the object pointed to by weakReference is deleted in between the call to get() and compute? (Or even during?)


#10

I think this only allocates if the message queue needs to reallocate. Usually if it’s being thrashed with uneven traffic. Maybe this could be improved to have a minimum guaranteed size to avoid allocating except in the worst circumstances?


#11

Yes @dave96 you‘re right about the weak reference loop, you would need to keep a lock if you know another thread might delete the object, I just wanted to show the overhead of resolving the pointers.


#12

Something with the same purpose of this was discussed here: Class for detecting unsafe operations in realtime contexts
which in turn is a spin-off of this other topic: Calling setValueNotifyingHost() from processBlock()

I think you should check those out because there are some very interesting considerations there.

In particular, I remember a consideration that struck me: like you did, my proposal was also to collect the IDs of those threads that are used for calling processBlock() and check agains those.

It was pointed out that it is entirely possible that the thread ids are reused, or that those same threads that for some time are allocated to audio processing, are later on used for other non-realtime stuff, and that would trigger any sort of false positive.

(you may deem such possibility as very improbable, but the #1 rule is to not make assumptions on how a DAW will behave. It’s almost the Murphy’s Law of Audio Plug-in Programming)

So, the proposal made by @jules turned out to be much more clean in my opinion, and it was simply to have a thread-local flag that is set right before every processBlock() call, and cleared immediately after it has returned. Kinda like a “scoped flag”.

At that point, “realtime-forbidden” operations should only check for that flag: if it is found to be set, then the operation is being performed in a realtime audio context and that should trigger the code that warns the developer, whatever it is (assertion, warning, etc.)


#13

so, basically this thing: https://docs.juce.com/master/classScopedValueSetter.html being used with a public class member (probably a bool)?


#14

If it’s lock free.

In C++11 you can only check this at runtime using std::atomic_is_lock_free, but C++17 lets you do this at compile time with std::atomic::is_always_lock_free.

That being said, whenever I’ve checked on the common OSs, the primitive types have been lock free.


#15

some of 'em aren’t lock free! I showed the source for std::atomic<shared_ptr> here, which is not lock free. just a head’s up.


#16

shared_ptr is definitely not a primitive type! I was suggesting that you’ll probably be OK on mainstream operating systems for ints, floats, bools and pointers.


#17

If you don’t need an actual bool you can use std::atomic_flag which is guaranteed lock free anywhere.


#18

Possibly, but remember that the flag must be local to the realtime audio thread, so a simple bool will not do: if another thread is (legitimately) doing some UI work in the same interval of time when the audio thread is in processBlock(), having a simple bool will result in false positives because its value will be seen as true in both threads.

You should probably use ThreadLocalValue, but I’m not sure it can be used in combination with ScopedValueSetter.


#19

I’ve changed the implementation to a scoped based approach, so in the destructor of the AudioThreadGuard(), the current thread ID is removed. I added this a while ago to make the AudioThreadGuard work on non-audio threads too - if you’re acquiring the audio lock on a non-audio thread, the same rules apply to the thread, so you can add a AudioThreadGuard there and it will add the thread id to the list of „hot“ threads.

This solution should also fix the behaviour of hosts you mentioned