Class for detecting unsafe operations in realtime contexts

Audio Plugins
1

Following-up the discussion started in this other topic, I’ve had a go at trying to implement a feature proposed there by @jules .

The intention is to mark the sections of code that are intended for realtime execution (e.g. audio callbacks in plug-ins), and to provide a way for checking whether the current thread is inside one of those.

This way, it will be possible to scatter jasserts in parts of code that are NOT safe for realtime usage, detecting whether they unintentionally end up being called by the audio threads.

The core of the feature is implemented by the following class.
I’ve intentionally NOT named it after something audio-related for two reasons:

a) It’s not mandatory that realtime-sensitive contexts are audio-related only
b) for the check to be available everywhere in JUCE and downstream, this should be added to the juce_core module, hence explicitly mentioning audio there would seem awkward.

class RealtimeSection
{
public:
    RealtimeSection()
    {
        RealtimeSection::getRecursionLevel() = RealtimeSection::getRecursionLevel().get() + 1;
    }

    ~RealtimeSection()
    {
        RealtimeSection::getRecursionLevel() = RealtimeSection::getRecursionLevel().get() - 1;
    }

    static bool isCurrentThreadInside()
    {
        return (RealtimeSection::getRecursionLevel().get() > 0);
    }

private:
    static ThreadLocalValue <int>& getRecursionLevel()
    {
        static ThreadLocalValue <int> recursionLevel;
        return recursionLevel;
    }
};

Usage is simple: a RealtimeSection object should be created on the stack at the beginning of the audio callback of each plug-in format wrapper.

Then, checking that non-realtime-safe code is not unintentionally executed by the audio thread is simply done with:

jassert (RealtimeSection::isCurrentThreadInside() == false);

What do you think?

7 Likes
2

this is probably my newbness showing itself. Would the intention be more obvious that you’re trying to update the recursionLevel variable if you changed the signature for your static method to this:

static ThreadLocalValue<int>* getRecursionLevel() 
{
    static ThreadLocalValue<int> recursionLevel;
    return &recursionLevel;
}

and the usage then became:

*RealtimeSection::getRecursionLevel() = RealtimeSection::getRecursionLevel()->get() + 1;

I’m a big fan of being able to see intention immediately in code with either the syntax used or the variable names. If I didn’t take a look at the API for ThreadLocalValue, your class would have been very confusing to understand the intent and mechanics. I understand that you’re taking advantage of this method a lot to do your work:

Type& ThreadLocalValue< Type >::operator*() const
Returns a reference to this thread's instance of the value.

however, the intention ends up being hidden when syntax like what you wrote gets used. For me, when I see function() = function() + 1; I can’t tell that the author is trying to modify the internal value function() is statically providing. and I’m pretty sure ##c++general would consider that a code smell lol

thoughts?

3

Nope. Always choose a reference rather than a raw pointer if you can.

Raw pointers create ambiguity about ownership of the object, and whether it can be null or not. Modern C++ style generally advises the avoidance of raw pointers at all cost.

1 Like
4

ok but function() = function() + 1 is kinda awful. can we agree on that? it’s not obvious what the intention is.
something like

var = function() + 1;
updateFunction(var);

is much clearer, even if it introduces a temporary variable.

At the very least, don’t use a function that begins with “get” to set a value lol

5

I think what’s confusing you is that the code above is written in a bit of a cumbersome way. What you’d actually write would be more like

RealtimeSection()
{
    auto& level = RealtimeSection::getRecursionLevel();
    level = level.get() + 1;
}
6

Yes, @matkatmusic, I admit that my code above looks better after some polishing:

  • renamed getRecursionLevel() to getRecursionCounter(), to convey the meaning that it is not returning a primitive value like an int (which would look awkward on the left side of assignments), but rather an object that works as a counter for the recursion level.
    To make it extra clear, the word “Reference” could be appended, too, but it seemed overkill in this case since it’s just an internal method after all.

  • used a local variable recursionCounter in both constructor and destructor to avoid clutter and confusion in the increment and decrement operations happening there.
    The code in my first post was like that because my very first implementation used a static ThreadLocalValue member. When switching to the better practice of holding it in a static method, I simply replaced occurrences of the member with invocations to the method. My bad for not spotting this earlier. Also thanks @jules for reminding the usefulness of auto in this context.

  • added a jassert to catch the (seemingly impossible) case of decrementing the counter into the negatives. Certanly that would indicate a problem!

Apart from those, I side with jules regarding the pointer thing: I think that the above changes make the code clear enough not to require an explicit returning of a pointer which, as pointed out, creates ambiguity w.r.t. ownership and whether nullptr is a valid value. A reference in this case has none of those ambiguities and is therefore a better choice IMHO.

The code now looks like this (still it lacks a proper documentation block. Adding the simplest usage example there would be the final touch but I don’t have time at the moment):

    class RealtimeSection
    {
    public:
        RealtimeSection()
        {
            auto& recursionCounter = RealtimeSection::getRecursionCounter();
            recursionCounter = recursionCounter.get() + 1;
        }

        ~RealtimeSection()
        {
            auto& recursionCounter = RealtimeSection::getRecursionCounter();
            recursionCounter = recursionCounter.get() - 1;
            jassert (recursionCounter.get() >= 0);
        }

        static bool isCurrentThreadInside()
        {
            return (RealtimeSection::getRecursionCounter().get() > 0);
        }

    private:
        static ThreadLocalValue <int>& getRecursionCounter()
        {
            static ThreadLocalValue <int> recursionLevel;
            return recursionLevel;
        }
    };
2 Likes
7

One further consideration: because ThreadLocalValue itself performs an allocation on the heap the first time it is accessed from a new thread, I propose also adding the following method to the above class.

/** Preallocates the internal ThreadLocalValue object for the current
thread, so that even the first usage of RealtimeSection on this thread 
will not cause an allocation at a realtime-critical moment.

For example, for audio plug-ins this could be called by the wrapper
in the same context where prepareToPlay() is called.
That will work on the assumption that the thread performing said
initialization will be the same which will perform the processing.

If such assumption does not hold, this will cost a single allocation
for each thread doing the processing, which may be a tolerable behavior
for a Debug build but should probably be avoided in Release
*/
static void willBeUsedInCurrentThread()
{
    RealtimeSection::getRecursionCounter().get();
}
8

The problem i see with this, this may can lead people to have a false positive information, that their code is realtime safe, because it doesn’t assert, but I don’t think it’s wrong either.

When i ever had realtime problems, this was never caused by performance intrusion or memory allocations, it were always different things.

I would appreciate if juce has finished patterns that solve specific performance/realtime problems.

  • clever scoped time measures objects, which write logs with minimum/maximum/average timing, which can used all around the code to observe potential problems

  • a multipurpose realtime allocator, which can be used for non heavy weight objects

  • implement realtime-save versions of sendChangeMessage()/triggerAsyncUpdate and other functions like that

  • a read only valueTree ghost-copy which can be used for example in the audio-thread

things like that

1 Like
9

have you seen this:

https://juce.com/doc/classScopedTimeMeasurement

10

Yes