-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Let IMT influence TThreadedObject #1019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
If IMT is enabled, TThreadedOject should allocate as many slots as threads the pool has been set with.
| fModel.reset(Internal::TThreadedObjectUtils::Detacher<T>::Detach(new T(std::forward<ARGS>(args)...))); | ||
|
|
||
| if(ROOT::IsImplicitMTEnabled()) { | ||
| TThreadedObject<T>::fgMaxSlots = ROOT::GetImplicitMTPoolSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the value for each TThreadObject creation?
Also, in order for this line (and the next) to be thread safe fgMaxSlots needs to be an std::atomic. [Otherwise, per the standard, the behavior when the reading (line 167) and the writing (line 164) happen at the 'same time' is undefined]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Will look into it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcanal doesn't everything go bonkers anyway if users call EnableImplicitMT in a thread while they are using implicit-mt-aware features in other threads concurrently?
In other words, shouldn't EnableImplicitMT be called strictly before doing anything that checks whether implicit-mt is enabled or not?
|
Starting build on |
|
Does fgMaxSlots have to be static? I can't figure out the reason apart from wanting to keep the slot size heterogeneous at any point of the program. I would need it to be local for dependency on IMT behaviours to make sense. |
|
@dpiparo What do we make of this? It needs a refresh, but is this the right approach performance wise? |
1 similar comment
|
@dpiparo what do you want to do with this one ? |
|
IIRC, not relevant anymore. Closing. |
If IMT is enabled, TThreadedObject should allocate as many slots as threads the pool has been set with.
This PR will be updated with another default value for fgMaxSlots once PR #1018 has been agreed upon and merged.