-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[core] Make TROOT::GetFunction more thread-safe #23222
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1751,27 +1751,19 @@ TObject *TROOT::GetFunction(const char *name) const | |
| if (!name || !*name) | ||
| return nullptr; | ||
|
|
||
| static std::atomic<bool> isInited = false; | ||
| // Look for function name in list of ROOT functions. Use the locking pattern | ||
| // with ROOT re-entrant locks to apply the following strategy: | ||
| // - First, early return if function is already available | ||
| // - Second, acquire a write lock and make other threads wait until all ROOT | ||
| // standard functions have been initialized through ProcessLine | ||
|
|
||
| // Capture the state before calling FindObject as it could change | ||
| // between the end of FindObject and the if statement | ||
| bool wasInited = isInited.load(); | ||
|
|
||
| auto f1 = fFunctions->FindObject(name); | ||
| if (f1 || wasInited) | ||
| R__READ_LOCKGUARD(ROOT::gCoreMutex); | ||
| if (auto f1 = fFunctions->FindObject(name)) | ||
| return f1; | ||
|
|
||
| // If 2 threads gets here at the same time, the static initialization "lock" | ||
| // will stall one of them until ProcessLine is finished and both will return the | ||
| // correct answer. | ||
| // Note: if one (or more) thread(s) is suspended right after the 'isInited.load()` | ||
| // and restart after this thread has finished the initialization (i.e. a rare case), | ||
| // the only penalty we pay is a spurious 2nd lookup for an unknown function. | ||
| [[maybe_unused]] static const auto _res = []() { | ||
| gROOT->ProcessLine("TF1::InitStandardFunctions(); TF2::InitStandardFunctions(); TF3::InitStandardFunctions();"); | ||
| isInited = true; | ||
| return true; | ||
| }(); | ||
| R__WRITE_LOCKGUARD(ROOT::gCoreMutex); | ||
| gROOT->ProcessLine("TF1::InitStandardFunctions(); TF2::InitStandardFunctions(); TF3::InitStandardFunctions();"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why has this to be a call to the interpreter? Can we break the dependency in another way?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. apologies, it is not strictly related to these changes, it has always been like that, but it became very visible with this PR...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Those code paths are "rare" in the original code (protected by the isInited) and thus does not really benefit from the pattern of taking a longer lock to avoid multiple smaller locks). |
||
|
|
||
| return fFunctions->FindObject(name); | ||
| } | ||
|
|
||
|
|
||
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.
fFunctionsis already a thread safe container and does not 'need' external locking