Consolidate loggability checks and replace streams.
Currently we check if a message should be printed at the call site using LogMessage::Loggable, in the LogMessage itself using LogMessage::IsNoop and in LogMessage::OutputToDebug using log_to_stderr_.
This change unifies the first two of these into a early return in Log().
Bug: webrtc:8982
Change-Id: Ia4e3e12b34716d76c05807e44db1ed4a62dffb87
Reviewed-on: https://webrtc-review.googlesource.com/97440
Commit-Queue: Jonas Olsson <jonasolsson@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24547}diff --git a/rtc_base/logging.cc b/rtc_base/logging.cc
index c386600..42f4fa9 100644
--- a/rtc_base/logging.cc
+++ b/rtc_base/logging.cc
@@ -64,16 +64,6 @@
return (end1 > end2) ? end1 + 1 : end2 + 1;
}
-std::ostream& GetNoopStream() {
- class NoopStreamBuf : public std::streambuf {
- public:
- int overflow(int c) override { return c; }
- };
- static NoopStreamBuf noop_buffer;
- static std::ostream noop_stream(&noop_buffer);
- return noop_stream;
-}
-
// Global lock for log subsystem, only needed to serialize access to streams_.
CriticalSection g_log_crit;
} // namespace
@@ -108,11 +98,7 @@
LoggingSeverity sev,
LogErrorContext err_ctx,
int err)
- : severity_(sev), is_noop_(IsNoop(sev)) {
- // If there's no need to do any work, let's not :)
- if (is_noop_)
- return;
-
+ : severity_(sev) {
if (timestamp_) {
// Use SystemTimeMillis so that even if tests use fake clocks, the timestamp
// in log messages represents the real system time.
@@ -120,14 +106,14 @@
// Also ensure WallClockStartTime is initialized, so that it matches
// LogStartTime.
WallClockStartTime();
- print_stream_ << "[" << std::setfill('0') << std::setw(3) << (time / 1000)
- << ":" << std::setw(3) << (time % 1000) << std::setfill(' ')
+ print_stream_ << "[" << rtc::LeftPad('0', 3, rtc::ToString(time / 1000))
+ << ":" << rtc::LeftPad('0', 3, rtc::ToString(time % 1000))
<< "] ";
}
if (thread_) {
PlatformThreadId id = CurrentThreadId();
- print_stream_ << "[" << std::dec << id << "] ";
+ print_stream_ << "[" << id << "] ";
}
if (file != nullptr) {
@@ -184,10 +170,8 @@
LoggingSeverity sev,
const char* tag)
: LogMessage(file, line, sev, ERRCTX_NONE, 0 /* err */) {
- if (!is_noop_) {
- tag_ = tag;
- print_stream_ << tag << ": ";
- }
+ tag_ = tag;
+ print_stream_ << tag << ": ";
}
#endif
@@ -199,21 +183,13 @@
LoggingSeverity sev,
const std::string& tag)
: LogMessage(file, line, sev) {
- if (!is_noop_)
- print_stream_ << tag << ": ";
+ print_stream_ << tag << ": ";
}
LogMessage::~LogMessage() {
- if (is_noop_)
- return;
-
FinishPrintStream();
- // TODO(tommi): Unfortunately |ostringstream::str()| always returns a copy
- // of the constructed string. This means that we always end up creating
- // two copies here (one owned by the stream, one by the return value of
- // |str()|). It would be nice to switch to something else.
- const std::string str = print_stream_.str();
+ const std::string str = print_stream_.Release();
if (severity_ >= g_dbg_sev) {
#if defined(WEBRTC_ANDROID)
@@ -237,18 +213,12 @@
void LogMessage::AddTag(const char* tag) {
#ifdef WEBRTC_ANDROID
- if (!is_noop_) {
- tag_ = tag;
- }
+ tag_ = tag;
#endif
}
-std::ostream& LogMessage::stream() {
- return is_noop_ ? GetNoopStream() : print_stream_;
-}
-
-bool LogMessage::Loggable(LoggingSeverity sev) {
- return sev >= g_min_sev;
+rtc::StringBuilder& LogMessage::stream() {
+ return print_stream_;
}
int LogMessage::GetMinLogSeverity() {
@@ -476,22 +446,23 @@
// static
bool LogMessage::IsNoop(LoggingSeverity severity) {
- if (severity >= g_dbg_sev)
+ if (severity >= g_dbg_sev || severity >= g_min_sev)
return false;
// TODO(tommi): We're grabbing this lock for every LogMessage instance that
// is going to be logged. This introduces unnecessary synchronization for
// a feature that's mostly used for testing.
CritScope cs(&g_log_crit);
- return streams_.size() == 0;
+ if (streams_.size() > 0)
+ return false;
+
+ return true;
}
void LogMessage::FinishPrintStream() {
- if (is_noop_)
- return;
if (!extra_.empty())
print_stream_ << " : " << extra_;
- print_stream_ << std::endl;
+ print_stream_ << "\n";
}
namespace webrtc_logging_impl {
@@ -525,6 +496,12 @@
return;
}
}
+
+ if (LogMessage::IsNoop(meta.meta.Severity())) {
+ va_end(args);
+ return;
+ }
+
LogMessage log_message(meta.meta.File(), meta.meta.Line(),
meta.meta.Severity(), meta.err_ctx, meta.err);
if (tag) {
@@ -564,7 +541,8 @@
log_message.stream() << *va_arg(args, const std::string*);
break;
case LogArgType::kVoidP:
- log_message.stream() << va_arg(args, const void*);
+ log_message.stream() << rtc::ToHex(
+ reinterpret_cast<uintptr_t>(va_arg(args, const void*)));
break;
default:
RTC_NOTREACHED();
diff --git a/rtc_base/logging.h b/rtc_base/logging.h
index b5af959..5f2aadb 100644
--- a/rtc_base/logging.h
+++ b/rtc_base/logging.h
@@ -57,6 +57,7 @@
#include "rtc_base/constructormagic.h"
#include "rtc_base/deprecation.h"
+#include "rtc_base/strings/string_builder.h"
#include "rtc_base/system/inline.h"
#include "rtc_base/thread_annotations.h"
@@ -407,18 +408,7 @@
void AddTag(const char* tag);
- static bool Loggable(LoggingSeverity sev);
-
- // Same as the above, but using a template argument instead of a function
- // argument. (When the logging severity is statically known, passing it as a
- // template argument instead of as a function argument saves space at the
- // call site.)
- template <LoggingSeverity S>
- RTC_NO_INLINE static bool Loggable() {
- return Loggable(S);
- }
-
- std::ostream& stream();
+ rtc::StringBuilder& stream();
// Returns the time at which this function was called for the first time.
// The time will be used as the logging start time.
@@ -464,6 +454,12 @@
// Useful for configuring logging from the command line.
static void ConfigureLogging(const char* params);
+ // Checks the current global debug severity and if the |streams_| collection
+ // is empty. If |severity| is smaller than the global severity and if the
+ // |streams_| collection is empty, the LogMessage will be considered a noop
+ // LogMessage.
+ static bool IsNoop(LoggingSeverity severity);
+
private:
friend class LogMessageForTesting;
typedef std::pair<LogSink*, LoggingSeverity> StreamAndSeverity;
@@ -481,18 +477,12 @@
static void OutputToDebug(const std::string& msg, LoggingSeverity severity);
#endif
- // Checks the current global debug severity and if the |streams_| collection
- // is empty. If |severity| is smaller than the global severity and if the
- // |streams_| collection is empty, the LogMessage will be considered a noop
- // LogMessage.
- static bool IsNoop(LoggingSeverity severity);
-
// Called from the dtor (or from a test) to append optional extra error
// information to the log stream and a newline character.
void FinishPrintStream();
- // The ostream that buffers the formatted message before output
- std::ostringstream print_stream_;
+ // The stringbuilder that buffers the formatted message before output
+ rtc::StringBuilder print_stream_;
// The severity level of this message
LoggingSeverity severity_;
@@ -506,8 +496,6 @@
// the message before output.
std::string extra_;
- const bool is_noop_;
-
// The output streams and their associated severities
static StreamList streams_;
@@ -527,13 +515,11 @@
// DEPRECATED.
// TODO(bugs.webrtc.org/9278): Remove once there are no more users.
#define RTC_LOG_SEVERITY_PRECONDITION(sev) \
- !(rtc::LogMessage::Loggable(sev)) \
+ (rtc::LogMessage::IsNoop(sev)) \
? static_cast<void>(0) \
: rtc::webrtc_logging_impl::LogMessageVoidify()&
#define RTC_LOG(sev) \
- for (bool do_log = rtc::LogMessage::Loggable<rtc::sev>(); do_log; \
- do_log = false) \
rtc::webrtc_logging_impl::LogCall() & \
rtc::webrtc_logging_impl::LogStreamer<>() \
<< rtc::webrtc_logging_impl::LogMetadata(__FILE__, __LINE__, \
@@ -541,7 +527,6 @@
// The _V version is for when a variable is passed in.
#define RTC_LOG_V(sev) \
- for (bool do_log = rtc::LogMessage::Loggable(sev); do_log; do_log = false) \
rtc::webrtc_logging_impl::LogCall() & \
rtc::webrtc_logging_impl::LogStreamer<>() \
<< rtc::webrtc_logging_impl::LogMetadata(__FILE__, __LINE__, sev)
@@ -564,8 +549,6 @@
}
#define RTC_LOG_E(sev, ctx, err) \
- for (bool do_log = rtc::LogMessage::Loggable<rtc::sev>(); do_log; \
- do_log = false) \
rtc::webrtc_logging_impl::LogCall() & \
rtc::webrtc_logging_impl::LogStreamer<>() \
<< rtc::webrtc_logging_impl::LogMetadataErr { \
@@ -603,7 +586,6 @@
} // namespace webrtc_logging_impl
#define RTC_LOG_TAG(sev, tag) \
- for (bool do_log = rtc::LogMessage::Loggable(sev); do_log; do_log = false) \
rtc::webrtc_logging_impl::LogCall() & \
rtc::webrtc_logging_impl::LogStreamer<>() \
<< rtc::webrtc_logging_impl::LogMetadataTag { \
diff --git a/rtc_base/logging_unittest.cc b/rtc_base/logging_unittest.cc
index af51212..263e992 100644
--- a/rtc_base/logging_unittest.cc
+++ b/rtc_base/logging_unittest.cc
@@ -150,7 +150,6 @@
: LogMessage(file, line, sev, err_ctx, err) {}
const std::string& get_extra() const { return extra_; }
- bool is_noop() const { return is_noop_; }
#if defined(WEBRTC_ANDROID)
const char* get_tag() const { return tag_; }
#endif
@@ -163,10 +162,7 @@
RTC_DCHECK(!is_finished_);
is_finished_ = true;
FinishPrintStream();
- std::string ret = print_stream_.str();
- // Just to make an error even more clear if the stream gets used after this.
- print_stream_.clear();
- return ret;
+ return print_stream_.Release();
}
private:
@@ -303,7 +299,6 @@
TEST(LogTest, CheckExtraErrorField) {
LogMessageForTesting log_msg("some/path/myfile.cc", 100, LS_WARNING,
ERRCTX_ERRNO, 0xD);
- ASSERT_FALSE(log_msg.is_noop());
log_msg.stream() << "This gets added at dtor time";
const std::string& extra = log_msg.get_extra();
@@ -314,7 +309,6 @@
TEST(LogTest, CheckFilePathParsed) {
LogMessageForTesting log_msg("some/path/myfile.cc", 100, LS_INFO);
- ASSERT_FALSE(log_msg.is_noop());
log_msg.stream() << "<- Does this look right?";
const std::string stream = log_msg.GetPrintStream();
@@ -340,21 +334,6 @@
}
#endif
-TEST(LogTest, CheckNoopLogEntry) {
- if (LogMessage::GetLogToDebug() <= LS_SENSITIVE) {
- printf("CheckNoopLogEntry: skipping. Global severity is being overridden.");
- return;
- }
-
- // Logging at LS_SENSITIVE severity, is by default turned off, so this should
- // be treated as a noop message.
- LogMessageForTesting log_msg("some/path/myfile.cc", 100, LS_SENSITIVE);
- log_msg.stream() << "Should be logged to nowhere.";
- EXPECT_TRUE(log_msg.is_noop());
- const std::string stream = log_msg.GetPrintStream();
- EXPECT_TRUE(stream.empty());
-}
-
// Test the time required to write 1000 80-character logs to a string.
TEST(LogTest, Perf) {
std::string str;
@@ -363,10 +342,7 @@
const std::string message(80, 'X');
{
- // Just to be sure that we're not measuring the performance of logging
- // noop log messages.
LogMessageForTesting sanity_check_msg(__FILE__, __LINE__, LS_SENSITIVE);
- ASSERT_FALSE(sanity_check_msg.is_noop());
}
// We now know how many bytes the logging framework will tag onto every msg.
diff --git a/rtc_base/stringutils.cc b/rtc_base/stringutils.cc
index bef128c..6baddd1 100644
--- a/rtc_base/stringutils.cc
+++ b/rtc_base/stringutils.cc
@@ -146,4 +146,10 @@
return std::string(buffer);
}
+std::string LeftPad(char padding, unsigned length, std::string s) {
+ if (s.length() >= length)
+ return s;
+ return std::string(length - s.length(), padding) + s;
+}
+
} // namespace rtc
diff --git a/rtc_base/stringutils.h b/rtc_base/stringutils.h
index d92ba02..367a673 100644
--- a/rtc_base/stringutils.h
+++ b/rtc_base/stringutils.h
@@ -329,6 +329,9 @@
// TODO(jonasolsson): replace with absl::Hex when that becomes available.
std::string ToHex(const int i);
+
+std::string LeftPad(char padding, unsigned length, std::string s);
+
} // namespace rtc
#endif // RTC_BASE_STRINGUTILS_H_
