Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve RootHandlers #23186

Merged
merged 4 commits into from
May 11, 2018
Merged

Improve RootHandlers #23186

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
529b3d9
simplify message checking
kpedro88 May 10, 2018
9cadac1
allow ignoring any level
kpedro88 May 10, 2018
6783e89
const vectors outside of fn
kpedro88 May 10, 2018
15bcb98
use constexpr, array, c-string
kpedro88 May 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
110 changes: 60 additions & 50 deletions FWCore/Services/plugins/InitRootHandlers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
#include <cstring>
#include <poll.h>
#include <atomic>
#include <algorithm>
#include <vector>
#include <string>

// WORKAROUND: At CERN, execv is replaced with a non-async-signal safe
// version. This can break our stack trace printer. Avoid this by
Expand Down Expand Up @@ -101,7 +104,7 @@ namespace edm {
private:
static char *const *getPstackArgv();
void enableWarnings_() override;
void ignoreWarnings_() override;
void ignoreWarnings_(edm::RootHandlers::SeverityLevel level) override;
void willBeUsingThreads() override;

void cachePidInfo();
Expand Down Expand Up @@ -142,38 +145,34 @@ namespace edm {
}

namespace {
enum class SeverityLevel {
kInfo,
kWarning,
kError,
kSysError,
kFatal
};

thread_local bool s_ignoreWarnings = false;
thread_local edm::RootHandlers::SeverityLevel s_ignoreWarnings = edm::RootHandlers::SeverityLevel::kInfo;

bool s_ignoreEverything = false;

bool find_if_string(const std::string& search, const std::vector<std::string>& substrs){
return (find_if(substrs.begin(), substrs.end(), [&search](const std::string& s) -> bool { return (search.find(s) != std::string::npos); }) != substrs.end());
}

void RootErrorHandlerImpl(int level, char const* location, char const* message) {

bool die = false;

// Translate ROOT severity level to MessageLogger severity level

SeverityLevel el_severity = SeverityLevel::kInfo;
edm::RootHandlers::SeverityLevel el_severity = edm::RootHandlers::SeverityLevel::kInfo;

if (level >= kFatal) {
el_severity = SeverityLevel::kFatal;
el_severity = edm::RootHandlers::SeverityLevel::kFatal;
} else if (level >= kSysError) {
el_severity = SeverityLevel::kSysError;
el_severity = edm::RootHandlers::SeverityLevel::kSysError;
} else if (level >= kError) {
el_severity = SeverityLevel::kError;
el_severity = edm::RootHandlers::SeverityLevel::kError;
} else if (level >= kWarning) {
el_severity = s_ignoreWarnings ? SeverityLevel::kInfo : SeverityLevel::kWarning;
el_severity = edm::RootHandlers::SeverityLevel::kWarning;
}

if(s_ignoreEverything) {
el_severity = SeverityLevel::kInfo;
if (s_ignoreEverything || el_severity <= s_ignoreWarnings) {
el_severity = edm::RootHandlers::SeverityLevel::kInfo;
}

// Adapt C-strings to std::strings
Expand Down Expand Up @@ -217,49 +216,60 @@ namespace {
&& (el_message.find("fill branch") != std::string::npos)
&& (el_message.find("address") != std::string::npos)
&& (el_message.find("not set") != std::string::npos)) {
el_severity = SeverityLevel::kFatal;
el_severity = edm::RootHandlers::SeverityLevel::kFatal;
}

if ((el_message.find("Tree branches") != std::string::npos)
&& (el_message.find("different numbers of entries") != std::string::npos)) {
el_severity = SeverityLevel::kFatal;
el_severity = edm::RootHandlers::SeverityLevel::kFatal;
}


// Intercept some messages and downgrade the severity

if ((el_message.find("no dictionary for class") != std::string::npos) ||
(el_message.find("already in TClassTable") != std::string::npos) ||
(el_message.find("matrix not positive definite") != std::string::npos) ||
(el_message.find("not a TStreamerInfo object") != std::string::npos) ||
(el_message.find("Problems declaring payload") != std::string::npos) ||
(el_message.find("Announced number of args different from the real number of argument passed") != std::string::npos) || // Always printed if gDebug>0 - regardless of whether warning message is real.
(el_location.find("Fit") != std::string::npos) ||
(el_location.find("TDecompChol::Solve") != std::string::npos) ||
(el_location.find("THistPainter::PaintInit") != std::string::npos) ||
(el_location.find("TUnixSystem::SetDisplay") != std::string::npos) ||
(el_location.find("TGClient::GetFontByName") != std::string::npos) ||
(el_location.find("Inverter::Dinv") != std::string::npos) ||
(el_message.find("nbins is <=0 - set to nbins = 1") != std::string::npos) ||
(el_message.find("nbinsy is <=0 - set to nbinsy = 1") != std::string::npos) ||
(level < kError and
(el_location.find("CINTTypedefBuilder::Setup")!= std::string::npos) and
(el_message.find("possible entries are in use!") != std::string::npos))) {
el_severity = SeverityLevel::kInfo;
std::vector<std::string> in_message = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is requiring the construction of this vector each time RootErrorHandlerImpl is called. Move this outside of the function and declare it const.
Do this for all the std::vector<std::string>.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, better to change this to const std::vector<const char* const> since no memory copies would then be needed to create std::string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I use direct list initialization instead of copy list initialization, is it still necessary to use char arrays?

Copy link
Contributor

Choose a reason for hiding this comment

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

Heck, it might be possible to completely do away with any allocations by using constexpr std::array<const char* const, ...>

Copy link
Contributor

Choose a reason for hiding this comment

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

To follow the standard, the system has to malloc both the std::vector and all the std::strings each call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But moving them outside of the function should avoid that, right? Does direct list initialization still entail memory copies?

"no dictionary for class",
"already in TClassTable",
"matrix not positive definite",
"not a TStreamerInfo object",
"Problems declaring payload",
"Announced number of args different from the real number of argument passed", // Always printed if gDebug>0 - regardless of whether warning message is real.
"nbins is <=0 - set to nbins = 1",
"nbinsy is <=0 - set to nbinsy = 1"
};

std::vector<std::string> in_location = {
"Fit",
"TDecompChol::Solve",
"THistPainter::PaintInit",
"TUnixSystem::SetDisplay",
"TGClient::GetFontByName",
"Inverter::Dinv"
};

if (find_if_string(el_message,in_message) ||
find_if_string(el_location,in_location) ||
(level < kError and (el_location.find("CINTTypedefBuilder::Setup")!= std::string::npos) and (el_message.find("possible entries are in use!") != std::string::npos)))
{
el_severity = edm::RootHandlers::SeverityLevel::kInfo;
}

// These are a special case because we do not want them to
// be fatal, but we do want an error to print.
std::vector<std::string> in_message_print = {
"number of iterations was insufficient",
"bad integrand behavior",
"integral is divergent, or slowly convergent"
};
bool alreadyPrinted = false;
if ((el_message.find("number of iterations was insufficient") != std::string::npos) ||
(el_message.find("bad integrand behavior") != std::string::npos) ||
(el_message.find("integral is divergent, or slowly convergent") != std::string::npos)) {
el_severity = SeverityLevel::kInfo;
if (find_if_string(el_message,in_message_print))
{
el_severity = edm::RootHandlers::SeverityLevel::kInfo;
edm::LogError("Root_Error") << el_location << el_message;
alreadyPrinted = true;
}

if (el_severity == SeverityLevel::kInfo) {
if (el_severity == edm::RootHandlers::SeverityLevel::kInfo) {
// Don't throw if the message is just informational.
die = false;
} else {
Expand All @@ -285,15 +295,15 @@ namespace {
// but we leave the other code in just in case we change
// the criteria for throwing.
if (!alreadyPrinted) {
if (el_severity == SeverityLevel::kFatal) {
if (el_severity == edm::RootHandlers::SeverityLevel::kFatal) {
edm::LogError("Root_Fatal") << el_location << el_message;
} else if (el_severity == SeverityLevel::kSysError) {
} else if (el_severity == edm::RootHandlers::SeverityLevel::kSysError) {
edm::LogError("Root_Severe") << el_location << el_message;
} else if (el_severity == SeverityLevel::kError) {
} else if (el_severity == edm::RootHandlers::SeverityLevel::kError) {
edm::LogError("Root_Error") << el_location << el_message;
} else if (el_severity == SeverityLevel::kWarning) {
} else if (el_severity == edm::RootHandlers::SeverityLevel::kWarning) {
edm::LogWarning("Root_Warning") << el_location << el_message ;
} else if (el_severity == SeverityLevel::kInfo) {
} else if (el_severity == edm::RootHandlers::SeverityLevel::kInfo) {
edm::LogInfo("Root_Information") << el_location << el_message ;
}
}
Expand Down Expand Up @@ -918,12 +928,12 @@ namespace edm {

void
InitRootHandlers::enableWarnings_() {
s_ignoreWarnings =false;
s_ignoreWarnings = edm::RootHandlers::SeverityLevel::kInfo;
}

void
InitRootHandlers::ignoreWarnings_() {
s_ignoreWarnings = true;
InitRootHandlers::ignoreWarnings_(edm::RootHandlers::SeverityLevel level) {
s_ignoreWarnings = level;
}

void
Expand Down
20 changes: 14 additions & 6 deletions FWCore/Utilities/interface/RootHandlers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,19 @@
namespace edm {
class EventProcessor;
class RootHandlers {
public:
enum class SeverityLevel {
kInfo,
kWarning,
kError,
kSysError,
kFatal
};

private:
struct WarningSentry {
WarningSentry(RootHandlers* iHandler): m_handler(iHandler){
m_handler->ignoreWarnings_();
WarningSentry(RootHandlers* iHandler, SeverityLevel level): m_handler(iHandler){
m_handler->ignoreWarnings_(level);
};
~WarningSentry() {
m_handler->enableWarnings_();
Expand All @@ -19,20 +28,19 @@ namespace edm {
friend class edm::EventProcessor;

public:
RootHandlers () {}
virtual ~RootHandlers () {}

template<typename F>
void ignoreWarningsWhileDoing(F iFunc) {
WarningSentry sentry(this);
void ignoreWarningsWhileDoing(F iFunc, SeverityLevel level=SeverityLevel::kWarning) {
WarningSentry sentry(this,level);
iFunc();
}

private:
virtual void willBeUsingThreads() = 0;

virtual void enableWarnings_() = 0;
virtual void ignoreWarnings_() = 0;
virtual void ignoreWarnings_(SeverityLevel level) = 0;
};
} // end of namespace edm

Expand Down