Ability to set/pass a “name” to juce::ThreadPool’s juce::ThreadPool::ThreadPoolThread.
We have a few juce::ThreadPool(s) going and sometimes it’s really hard to figure which one of the juce::ThreadPool::ThreadPoolThread(s) we failed at, especially on release builds.
Ooh, good idea. The ThreadPool could have a name field, and the function ThreadPool::createThreads() could combine the index (String(i)) and the referenced ThreadPool’s name to provide a better name than “Pool” when creating each Thread.
yeah, i always end up patching juce because of that 
Here’s something we ended up doing recently triggered by a very painful macOS crash log debug session.
@jacobsologub nice, exactly what I was looking for. can you please create a pull request against the upstream JUCE repo?
can you please create a pull request against the upstream JUCE repo?
Why bother ?
I’ve already made a PR/MR internally I’ll keep you posted.
thanks!
I saw you rewrite the HighResolutionTimer thats why I can not find where the thread name is set, but before it was here: JUCE/modules/juce_core/threads/juce_HighResolutionTimer.cpp at 69795dc8e589a9eb5df251b6dd994859bf7b3fab · juce-framework/JUCE · GitHub
maybe you could also provide the ability to set a name when implement the HighResolutionTimer which is then used as a thread name?
So it’s here now. It’s not a bad idea but it’s a little more tricky. The fact it runs on a separate thread is really an implementation detail. On Windows for example we don’t create a thread, instead it uses a Multimedia Thread supplied by the OS (see here for details). In general I can see cases where we might decide to share threads between some instances based on performance needs and the system it’s running on. Maybe we could think of some other way to allow an ID for each timer. The only other thing that concerns me is how many timers are you actually running, in general I would probably advise against lots of HighResolutionTimer instances.
In general I can see cases where we might decide to share threads between some instances based on performance needs and the system it’s running on. Maybe we could think of some other way to allow an ID for each timer.
The main reason to add a name or ID to the timers are the crash reports or other reports where we can directly see which thread crashed the app. Sharing a thread between high frequent timers is not an option for use, but I also noticed that you provide a MultiTimer which we may can use for the less frequent calls.
The only other thing that concerns me is how many timers are you actually running, in general I would probably advise against lots of
HighResolutionTimerinstances.
Why? Currently we have 5 active HighResolutionTimer, another 10 which can be activated dynamically. It depends which view the user is opening. But not all at the same time. But even if they could, are 15 HighResolutionTimer instances too much?
The main reason to add a name or ID to the timers are the crash reports or other reports where we can directly see which thread crashed the app.
Absolutely but what I’m suggesting is that on at least Windows we just don’t have control of this, we can’t name the thread, and there is only ever one thread that is shared between all instances. If we do it any other way (and I tried) we loose a great deal of the performance.
I also noticed that you provide a MultiTimer which we may can use for the less frequent calls.
The MultiTimer is not high resolution, it’s like Timer, they both run on the message thread. In most cases though I would recommend you use them over HighResolutionTimer, only use HighResolutionTimer when you really need to.
are 15 HighResolutionTimer instances too much?
Windows has a limit of 16 so it’s hovering very close to too much. Basically waking up a thread is the expensive and time consuming bit so when you have lots of threads that are regularly being woken up you might find the performance of the timers dropping. Not to mention that these are high priority threads, and in the case of Windows it’s a thread servicing other multimedia tasks such as audio, so there is a higher risk of causing audio glitches. I wonder if some kind of MultiHighResolutionTimer running on a single thread could potentially be more performant in such a use case.
This is now on develop, thanks or reporting. Note we took a slightly different approach as the constructor was getting a little big to be honest. There is now a ThreadPoolOptions struct for building up the arguments.
Slightly OT: I might be missing something obvious here, but why are these SomeClassOptions classes not implemented as nested classes simply named Options inside their main SomeClass instead? (resulting in SomeClass::Options, to be clear)
So I went that route initially and the main thing that pulled us away is that we hit a compiler issue on clang and gcc, if we want to use a default constructor and try to construct an instance for a default parameter argument for the OuterClass. You can see what I mean here.
I did also consider once we created a separate class that we could use a using statement to define it in the inner class, but in the end ThreadPoolOptions is less characters than ThreadPool::Options so I didn’t see it being much of an advantage to users at that point.
Ah yes, now that I see the error message, I remember having encountered the same issue before in my code.
Honestly, that left me scratching my head because the condition seems to be fulfilled: the default member initializer for x is specified before the end of its enclosing class, isn’t it?
In the end, I think I did resolve it the same way as you, by extracting the inner class and making it a “sibling” of the outer.
Sadly, I couldn’t find any other workaround: for this issue, I had to proceed by trial and error because it was one of the cases in which I found it impossible to understand what the compiler was trying to tell me, and hence how to fix it.
All I can think is that the default constructor must actually be defined outside the class after the OuterClass, if you define the constructor it works. Although I like the idea of nested classes in principle it’s not uncommon I find they trip me up. For example you can’t forward declare a nested class.
There is no advantage but consistency with the rest of juce, it’s funny these things are not peer reviewed and rejected as inconsistent with the library practices by the juce team.
To be completely clear we do peer review, and consistency is a consideration, however none of us claim to be infallible, mistakes will happen which is one of the reasons to get it onto the develop branch first.
Although we try to minimise breaking changes on the main branch, we make no such promises on the develop branch (not to say it isn’t still a consideration). Therefore feedback on the matter is important and hopefully it means when mistakes do occur we can fix them with minimal impact.
Looking through the codebase I agree it would be more consistent to have this as an Inner class, and therefore likely an oversight on our behalf. We may have been blindsided by the issue we saw at the time.
I’ll chat to the team about this it might be that we just add a using declaration.
Maybe we could also consider adding something to the coding standards to state what we think is best practice when using this methodology. Generally we’ve only used this pattern in a few places but I think it is becoming more common place and IMO at least I do think builder structs like this are nicer than lots of constructors and/or constructor arguments, so being clear on how we adopt the pattern for consistency will likely be important going forward.
Im not sure what the critique point is here, weither the options class or the inner class, it I say stick with the options class! As you said, it is inarguably cleaner code. It also sets up a better future for even more parameters or more complex creation algorithms. If it doesn’t fit the rest of the code base yet, it’s probably time to start adopting this more modern design pattern, as you did.
The juce URL has a great example of options that can be assembled before sending it into a method.
Having internal classes avoid one common issue in huge frameworks that is namespace pollution by mostly similarly prefixed symbols which might or might not be related with each other. It is often under estimated.
