AsyncUpdater which don't malloc

doh ! i must have missed it for years ! and i’ve checked and rechecked the Component header files a lot of time…

[quote=“zamrate”]Well, the original JUCE way is that when I have many buttons in one app, I have to add some ButtonListener(s), and there will be code with many IF’s ELSE’s there. This is even worse than switches, because it is not optimised (switches use look-up tables) and it is by the way not more readable than switches IMHO.
Plus: adding 2 extra user-definable int’s to the Component class is not like really asking much. They could also be used for something else.[/quote]

Maybe I’m missing you point, but why not use the Component properties for what you want? efficiency of string to int conversion?

I’m not a fan of cascaded/if/else either. If the number of widgets to handle in such a listener callback were too big or just unknown at compile time, I’d register callbacks with an hash map

Yes, exactly.

Hi Jules,

I’ve talk a bit with Digi support, and they traced the issues I had to mallocing in the audio thread.

I’ve finally eradicate all mallocs from my audio thread including the AsyncUpdater I was using, using my own AsyncUpdater and the issue is now gone.(CPU overload)

So Protools definitely doesn’t like any malloc in the audio thread.
My problem is that we use the MIDI juce MidiKeyboardComponent which use internally triggerAsyncUpdate.

What about updating juce AsyncUpdater so it doesn’t malloc ? :smiley:

Here is the code I’m using in my own code

class AsyncUpdaterTimer : public juce::Timer
  {
  public:
    static AsyncUpdaterTimer &GetInstance()
    {
      static scoped_ptr<AsyncUpdaterTimer> obj(new AsyncUpdaterTimer());
      return (*obj);
    }
    
    AsyncUpdaterTimer() 
      : mFIFO(1024)
    {
      startTimer(1);
    }

    virtual void timerCallback()
    {
      while (!mFIFO.IsEmpty())
      {
        AsyncUpdater *pAsyncUpdater = NULL;
        mFIFO.Pop(pAsyncUpdater);

        ScopedLock lock(mLock);
        std::set<AsyncUpdater*>::const_iterator it = mListeners.find(pAsyncUpdater);
        if (it != mListeners.end())
        {
          pAsyncUpdater->handleUpdateNowIfNeeded();
        }
      }
    }

    void Push(AsyncUpdater* pAsyncUpdater)
    {
      mFIFO.Push(pAsyncUpdater);
    }

    void Register(AsyncUpdater* pAsyncUpdater)
    {
      ScopedLock lock(mLock);
      mListeners.insert(pAsyncUpdater);
    }

    void UnRegister(AsyncUpdater* pAsyncUpdater)
    {
      ScopedLock lock(mLock);
      mListeners.erase(pAsyncUpdater);
    }

  protected:
    CriticalSection mLock;
    LockFreeFIFO<AsyncUpdater*> mFIFO;
    std::set<AsyncUpdater*> mListeners;
  };

  AsyncUpdater::AsyncUpdater()
    : asyncMessagePending(false)
  {
    AsyncUpdaterTimer::GetInstance().Register(this);
  }

  AsyncUpdater::~AsyncUpdater()
  {
    AsyncUpdaterTimer::GetInstance().UnRegister(this);
  }

  void AsyncUpdater::triggerAsyncUpdate()
  {
    if (!asyncMessagePending)
    {
      asyncMessagePending = true;
      AsyncUpdaterTimer::GetInstance().Push(this);
    }
  }

  void AsyncUpdater::cancelPendingUpdate()
  {
    asyncMessagePending = false;
  }

  void AsyncUpdater::handleUpdateNowIfNeeded()
  {
    if (asyncMessagePending)
    {
      asyncMessagePending = false;
      handleAsyncUpdate();
    }
  }

Thanks,

Just a little reminder: The MIDI In stuff also has new/malloc’s.

Do you mean midi input device stuff? If so, I don’t see why that’d be a problem?

otristan, your code’s ok, but it’s really not the best way to implement it. Using a static list + singleton + clients that register with it is all just duplicating functionality that’s already there in the Timer class. But you’ve added all the complication and overhead of a queue that can (and probably will) overflow and lose messages.

Honestly, like I’ve been saying all along, just making each updater object a Timer is much simpler, more reliable, and probably faster.

Well I was not a huge fan of having one Timer per AsyncUpdater
as most of the time, the AsyncUpdater do not need to be triggered.

but if you don’t think it would be annoying then…

It seems that you use AsyncUpdater to notify from the audio thread
in your AudioProcessing class too, so your code will be more robust under Protools too :slight_smile:

Thanks,

Yes, I might update the class to work without sending messages.

True, your code would be better if there’s a really huge number of objects that fire rarely. I can’t help thinking there must be a simple solution that solves all these problems…

Do you mean midi input device stuff? If so, I don’t see why that’d be a problem?[/quote]

Sorry, I was remembering this slighty wrong. It was actually a delete in the MidiOutput (which deletes the message). I don’t know if freeing memory that has been allocated takes as long as allocating it, but still I would never call new/delete or malloc/free in any time-critical function. I just don’t trust the OS’es.

[quote=“jules”]Yes, I might update the class to work without sending messages.

True, your code would be better if there’s a really huge number of objects that fire rarely. I can’t help thinking there must be a simple solution that solves all these problems…[/quote]

Oh yeah that would be great.

Thanks,

[quote]
I can’t help thinking there must be a simple solution that solves all these problems…[/quote]

Jules, what about this idea : changing the implementation of postMessage() this way :

  • make a new version of postMessage() called “postMessageNoDel()” that copies the message (no new() required and no deleting). That version will of course copies all the message’s content in LockFreeFifo or using OS primitives
  • implements the current version of postMessage() (so old code won’t break) on top of that, deleting the message right away :

postMessage(Message* msg) { postMessageNoDel(msg); delete msg; }

  • Finally, use the new version where it’s really needed (AsyncUpdater, ChangeBroadcaster and some direct usage of postMessage()…)

What do you think of that ?

That doesn’t really help, because it’d still involve a call to the OS’s post message function, and there’s no way that that’ll be lock-free.

[quote=“jules”]Yes, I might update the class to work without sending messages.

True, your code would be better if there’s a really huge number of objects that fire rarely. I can’t help thinking there must be a simple solution that solves all these problems…[/quote]
see post below

Additionaly to my previous post it seems you already have something to do this
MessageManager::callFunctionOnMessageThread

What about using this ?

[quote=“otristan”]Additionaly to my previous post it seems you already have something to do this
MessageManager::callFunctionOnMessageThread

What about using this ?[/quote]

I suggested that too, but that function is synchronous, so it won’t do.

Another variant would be not to dispatch Message objects via OS messages, but only notify a FIFO that it has something to pickup via an OS message (which need not carry an allocated object).

So the FIFO output is checked at each iteration in the GUI thread and process the messages in it should there be any. In the FIFO input, a message object is placed (without a malloc), just for the GUI thread to start dispatching messages.

Problem with preallocated FIFOs is that you need to know the size of the objects in it. Subclasses of Message can have any size. But the message FIFO can allocate memory from a mem pool which is more optimized and does not propagate to a malloc/free, so should be fairly quick and painless (boost has a decent implementation).

Templatized versions of the FIFO could then direct behaviour, i.e. holding objects of a certain size, different sizes, use a mem pool or use OS malloc/free etc.

Indeed.
We only need a fire and forget version callFunctionOnMessageThread
that uses PostMessage instead of SendMessage on Windows for example.

On Windows:

void* MessageManager::asyncCallFunctionOnMessageThread (MessageCallbackFunction* callback, void* userData) { return (void*) PostMessage (juce_messageWindowHandle, specialCallbackId, (WPARAM) callback, (LPARAM) userData); }

And for the AsyncUpdater

[code] class AsyncUpdaterManager
{
public:
static AsyncUpdaterManager &GetInstance()
{
static scoped_ptr obj(new AsyncUpdaterManager());
return (*obj);
}

static void* Callback(void *user)
{
  AsyncUpdater* pAsyncUpdater = (AsyncUpdater*)user;

  ScopedLock lock(GetInstance().mLock);
  std::set<AsyncUpdater*>::const_iterator it = GetInstance().mListeners.find(pAsyncUpdater);
  if (it != GetInstance().mListeners.end())
  {
    pAsyncUpdater->handleUpdateNowIfNeeded();
  }
  return 0;
}

void Push(AsyncUpdater* pAsyncUpdater)
{
  juce::MessageManager::getInstance()->asyncCallFunctionOnMessageThread(&AsyncUpdaterManager::Callback, pAsyncUpdater);
}

void Register(AsyncUpdater* pAsyncUpdater)
{
  ScopedLock lock(mLock);
  mListeners.insert(pAsyncUpdater);
}

void UnRegister(AsyncUpdater* pAsyncUpdater)
{
  ScopedLock lock(mLock);
  mListeners.erase(pAsyncUpdater);
}

protected:
CriticalSection mLock;
std::set<AsyncUpdater*> mListeners;
};

AsyncUpdater::AsyncUpdater()
: asyncMessagePending(false)
{
AsyncUpdaterManager::GetInstance().Register(this);
}

AsyncUpdater::~AsyncUpdater()
{
AsyncUpdaterManager::GetInstance().UnRegister(this);
}

void AsyncUpdater::triggerAsyncUpdate()
{
if (!asyncMessagePending)
{
asyncMessagePending = true;
AsyncUpdaterManager::GetInstance().Push(this);
}
}

void AsyncUpdater::cancelPendingUpdate()
{
asyncMessagePending = false;
}

void AsyncUpdater::handleUpdateNowIfNeeded()
{
if (asyncMessagePending)
{
asyncMessagePending = false;
handleAsyncUpdate();
}
}[/code]

Sure there is some system call indirectly but it doesn’t malloc at all

You seem to have a bizarre trust in PostMessage! It’s not designed to be a real-time function. Even if you assume that the windows message queue is lock-free (and I can’t think why they’d bother going to the trouble of making it so), the queue isn’t a fixed size, so it must occasionally have to do a realloc when you post a mesage that won’t fit.

Right.

It seems the equivalent on OSX use EventRef which does some malloc for sure
so it not a possible solution indeed.

Still stucked…

hi,

any news concerning this issue?
that would great to get rid of memory allocation especially for people targetting RTAS.