Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions core/base/src/TROOT.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fFunctions is already a thread safe container and does not 'need' external locking

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();");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gROOT->ProcessLine provides its own lock and does not 'need' external locking.

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);
}

Expand Down
Loading