I just wonder if anyone from the JUCE team has an idea about this.
I currently have my own solution that stores the message thread id in a global atomic, but this would be better implemented on the JUCE side as this is called by the JUCE code and not just ‘user’ code.
A nice solution could be to just set a thread-local flag at the top of the message thread. It could be accessed from anywhere, but only in the message thread it’d be true.
Why not use a static thread_local boolean? There is already a method in place, that can mark the current thread (MessageManager::getInstance()->setCurrentThreadAsMessageThread())
This might not work as we have a setter for the message thread which can be used when driving custom message threads, and we can’t unset a previously set different message thread. An atomic should be the way to go, or we can’t call setThisThreadAsMessageThread more than once
I’ve given that another bit of thought, and I think a non-atomic thread_local boolean is still possible to be implemented into the current code base, if RAII is used to dynamically use a different thread as the message thread. Follow my reasoning here:
Thread A is the main application thread, setting the thread_local boolean to true at the very start of the application
Thread B wants to take over the message thread (for example using the MessageManagerLock) or any other way
Thread B now has to make sure, that Thread A is paused. This has nothing to do with the mechanics of how a thread knows it is currently the message thread.
While Thread B knows it is now the only one working the queue, it might as well set the boolean to true. Technically, there are now two threads that would report being the message thread. But the execution of Thread A is paused, so it doesn’t really matter what the boolean says, it cannot and should not be queried anyway.
Once Thread B is done working the message queue (or the lock goes out of scope), the boolean is set back to false.
The boolean is not meant to be any kind of sync mechanism between threads or the message queue. It should just assure that any piece of code can ask if it is currently save to execute code, that may only be executed on the message thread.
I can see a problem, when Thread A wants to permanently give away the message queue to a different thread and then (instead of being stopped and discarded) do completely different things. But is that possible/feasible/advisable to be implemented?
This would not work because the message thread can ‘become’ the message thread later in the life time of the application. with a thread local you would somehow need to keep track of the old message thread variable and set it to false, which is not possible.
I think just taking the current (non atomic) variable and make it atomic is the correct way to do this.
Thank you for sharing info about this. We’re keeping an eye on this request, and considering lock free changes.
It looks like this lock might be contested, so there is a theoretical possibility for thread rescheduling, which could be alleviated by using a spin lock. I’ll have to investigate this more to be certain though.
While this is a theoretical possibility, I wonder, have you ever observed this lock causing a wait/thread rescheduling in a profiler or other tool? If this is something that’s happening in practice, that’s increasing the severity of this ticket.
What is a preferred solution is to just store the ThreadID in an atomic, should be a pretty trivial change.
As for performance - it’s been a while since I’ve profiled this, but switching away from ParameterAttachment to a Timer based approach was indeed solving quite a lot of performance issues I was having, but that was a while ago.
I suspect project with heavy usage of ParameterAttachment/isThisTheMessageThread from the audio thread would also take a hit from the rescheduler due to the lock.
Also important to note, that this can only happen when the parameter is changed on the audio thread, correct? Might be why this hasn’t come up so often.
Parameters always change in the audio thread during automation… It’s super common. But yes, I would expect this to be more of an observable issue if you automate many parameters (or just run many attachments/checks connected to a single parameter).
Also that check is something quite a lot of library code that is built on top of JUCE has to use - definitely not just parameters. For example I wanted to use this to create a shared_ptr wrapper that never deletes objects in the audio thread - and in those cases there could be thousands of such checks/objects in a single block.
Just making the messageThreadIdMutex a SpinLock looks like the safest change for me.
Just using an atomic to store the thread ID would change the semantics inside setCurrentThreadAsMessageThread and the windows specific teardown/initialisation could now overlap with other accesses to the thread id.
No change has been made yet, just a discussion at this point. Upon closer inspection of the problem, a lock free solution to this seems non-trivial. So there is a possibility of introducing a race condition.
Unless of course we just use a SpinLock, but given that we haven’t demonstrably shown yet that this is the source of performance problems or the cause for thread rescheduling, we are taking the cautious route.
This seems like a scenario where a compile-time flag could use the simple and efficient TLS mechanism and if you need to be able to dynamically change the message thread you can disable that compile-time mechanism.
Or set an atomic flag when a message thread change is detected to fallback to the more robust locking mechanism. In any case if you’re changing the message thread dynamically then you’ve already got to worry about the result of “isThisTheMessageThread” being cached in a local variable and becoming untrue before the associated code branch is finished executing. It’s not like it’s holding that lock to ensure it remains true.
Along those lines, I wonder if it’s worth making a distinction between application API needs and internal framework needs. Like, maybe no need to rock the boat of thousands of apps and change how setCurrentThreadAsMessageThread checks internally, just a need from an application API point of view to have a worry-free check (that can pass RTSan).
Can you explain more on the Windows specific problems with atomics?
This seems like a trivial change to me and that ID is already stored in the shared bit of the class. I can’t see why an atomic would make this different.
All I’m seeing in code is:
void MessageManager::setCurrentThreadAsMessageThread()
{
auto thisThread = Thread::getCurrentThreadId();
const std::lock_guard<std::mutex> lock { messageThreadIdMutex };
if (std::exchange (messageThreadId, thisThread) != thisThread)
{
#if JUCE_WINDOWS
// This is needed on windows to make sure the message window is created by this thread
doPlatformSpecificShutdown();
doPlatformSpecificInitialisation();
#endif
}
}
Reading through the Windows code itself I see no reason for the lock - the MessageManager is a singleton and that operation only happens once.
Is the lock trying to protect against multiple threads calling setCurrentThreadAsMessageThread? I don’t think it’s possible with a singleton.
