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

[Logging] Add DMLC_NO_INLINE to LogMessageFatal #615

Merged
merged 4 commits into from
May 21, 2020
Merged

[Logging] Add DMLC_NO_INLINE to LogMessageFatal #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c2cea7
Move dmlc::LogMessageFatal to the heap
junrushao May 20, 2020
6d84163
[Logging] Add `DMLC_NO_INLINE` to `LogMessageFatal`
junrushao May 21, 2020
cda96e4
Merge branch 'master' into feature/2020-05-20/stack-usage-strikes-back
junrushao May 21, 2020
32acd5a
fix wrongly merged code
junrushao May 21, 2020
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
6 changes: 6 additions & 0 deletions include/dmlc/base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,12 @@ typedef unsigned __int64 uint64_t;
#define noexcept(a) noexcept_##a
#endif

#if defined(_MSC_VER)
#define DMLC_NO_INLINE __declspec(noinline)
#else
#define DMLC_NO_INLINE __attribute__((noinline))
#endif

#if DMLC_USE_CXX11
#define DMLC_THROW_EXCEPTION noexcept(false)
#define DMLC_NO_EXCEPTION noexcept(true)
Expand Down
58 changes: 34 additions & 24 deletions include/dmlc/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,7 @@ class LogCheckError {

#define CHECK_BINARY_OP(name, op, x, y) \
if (dmlc::LogCheckError _check_err = dmlc::LogCheck##name(x, y)) \
std::unique_ptr<dmlc::LogMessageFatal>( \
new dmlc::LogMessageFatal(__FILE__, __LINE__))->stream() \
dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \
<< "Check failed: " << #x " " #op " " #y << *(_check_err.str) << ": "

#pragma GCC diagnostic push
Expand All @@ -220,10 +219,9 @@ DEFINE_CHECK_FUNC(_NE, !=)
#pragma GCC diagnostic pop

// Always-on checking
#define CHECK(x) \
if (!(x)) \
std::unique_ptr<dmlc::LogMessageFatal>( \
new dmlc::LogMessageFatal(__FILE__, __LINE__))->stream() \
#define CHECK(x) \
if (!(x)) \
dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \
<< "Check failed: " #x << ": "
#define CHECK_LT(x, y) CHECK_BINARY_OP(_LT, <, x, y)
#define CHECK_GT(x, y) CHECK_BINARY_OP(_GT, >, x, y)
Expand Down Expand Up @@ -428,30 +426,42 @@ class LogMessageFatal : public LogMessage {
#else
class LogMessageFatal {
public:
LogMessageFatal(const char* file, int line) {
log_stream_ << "[" << pretty_date_.HumanDate() << "] " << file << ":"
<< line << ": ";
LogMessageFatal(const char *file, int line) {
Entry::ThreadLocal()->Init(file, line);
}
std::ostringstream &stream() { return log_stream_; }
~LogMessageFatal() DMLC_THROW_EXCEPTION {
std::ostringstream &stream() { return Entry::ThreadLocal()->log_stream; }
DMLC_NO_INLINE ~LogMessageFatal() DMLC_THROW_EXCEPTION {
Copy link
Member Author

Choose a reason for hiding this comment

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

The no-inline trick here instructs the compiler not to merge the complicated logic of throwing in the destructor into the caller. This usually wouldn't hurt performance because it is quite a slow path

#if DMLC_LOG_STACK_TRACE
log_stream_ << "\n" << StackTrace(1, LogStackTraceLevel()) << "\n";
#endif

// throwing out of destructor is evil
// hopefully we can do it here
// also log the message before throw
#if DMLC_LOG_BEFORE_THROW
LOG(ERROR) << log_stream_.str();
Entry::ThreadLocal()->log_stream << "\n"
<< StackTrace(1, LogStackTraceLevel())
<< "\n";
#endif
throw Error(log_stream_.str());
throw Entry::ThreadLocal()->Finalize();
Copy link
Member Author

Choose a reason for hiding this comment

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

We moved all the logic except throw itself to another class, to make sure the unsafe region is minimized

}

private:
std::ostringstream log_stream_;
DateLogger pretty_date_;
LogMessageFatal(const LogMessageFatal&);
void operator=(const LogMessageFatal&);
struct Entry {
std::ostringstream log_stream;
DMLC_NO_INLINE void Init(const char *file, int line) {
DateLogger date;
log_stream.str("");
log_stream.clear();
log_stream << "[" << date.HumanDate() << "] " << file << ":" << line
<< ": ";
}
dmlc::Error Finalize() {
#if DMLC_LOG_BEFORE_THROW
LOG(ERROR) << log_stream.str();
#endif
return dmlc::Error(log_stream.str());
}
DMLC_NO_INLINE static Entry *ThreadLocal() {
static thread_local Entry *result = new Entry();
Copy link
Member Author

@junrushao junrushao May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The introduction of TLS helps to reduce the size of class dmlc::LogMessageFatal to zero, which enables some opportunities in compiler optimization

return result;
}
};
LogMessageFatal(const LogMessageFatal &);
void operator=(const LogMessageFatal &);
};
#endif

Expand Down
11 changes: 11 additions & 0 deletions test/unittest/unittest_logging_throw.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright by Contributors
#define DMLC_LOG_FATAL_THROW 1

#include <dmlc/logging.h>
#include <gtest/gtest.h>

TEST(LoggingThrow, exception) {
EXPECT_THROW({
LOG(FATAL) << "message";
}, dmlc::Error);
}