From 0e523c7056645123efaced6635e381a7581f9045 Mon Sep 17 00:00:00 2001 From: Ivan Berg Date: Tue, 4 Jun 2024 11:42:54 -0700 Subject: [PATCH] Address PR comments --- .../onnxruntime/core/common/logging/isink.h | 10 ++--- .../onnxruntime/core/common/logging/logging.h | 13 +++--- .../core/common/logging/sink_types.h | 11 +++++ onnxruntime/core/common/logging/logging.cc | 41 ++++++++----------- .../common/logging/sinks/composite_sink.h | 4 +- .../core/platform/windows/logging/etw_sink.h | 4 +- onnxruntime/core/session/inference_session.cc | 14 +++++-- 7 files changed, 51 insertions(+), 46 deletions(-) create mode 100644 include/onnxruntime/core/common/logging/sink_types.h diff --git a/include/onnxruntime/core/common/logging/isink.h b/include/onnxruntime/core/common/logging/isink.h index a7d64a310fbcf..fd011e71611fc 100644 --- a/include/onnxruntime/core/common/logging/isink.h +++ b/include/onnxruntime/core/common/logging/isink.h @@ -6,17 +6,15 @@ #include #include "core/common/logging/logging.h" +#include "core/common/logging/sink_types.h" namespace onnxruntime { namespace logging { class ISink { public: - ISink() = default; + explicit ISink(SinkType type = SinkType::BaseSink) : type_(type) {} - enum SinkType { BaseSink, - CompositeSink, - EtwSink }; - virtual SinkType GetType() const { return BaseSink; } + SinkType GetType() const { return type_; } /** Sends the message to the sink. @@ -37,6 +35,8 @@ class ISink { virtual ~ISink() = default; private: + SinkType type_; + // Make Code Analysis happy by disabling all for now. Enable as needed. ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE(ISink); diff --git a/include/onnxruntime/core/common/logging/logging.h b/include/onnxruntime/core/common/logging/logging.h index c07a4e14a6bd3..6f8459599241b 100644 --- a/include/onnxruntime/core/common/logging/logging.h +++ b/include/onnxruntime/core/common/logging/logging.h @@ -16,6 +16,7 @@ #include "core/common/logging/capture.h" #include "core/common/logging/macros.h" #include "core/common/logging/severity.h" +#include "core/common/logging/sink_types.h" #include "core/platform/ort_mutex.h" #include "date/date.h" @@ -171,18 +172,16 @@ class LoggingManager final { */ static LoggingManager* GetDefaultInstance(); -#ifdef _WIN32 /** - Removes the ETW Sink if one is present + Removes a Sink if one is present */ - void RemoveEtwSink(); + void RemoveSink(SinkType sinkType); /** - Adds an ETW Sink to the current sink creating a CompositeSink if necessary - @param etwSeverity The severity level for the ETW Sink + Adds a Sink to the current sink creating a CompositeSink if necessary + @param severity The severity level for the new Sink */ - void AddEtwSink(logging::Severity etwSeverity); -#endif + void AddSink(SinkType sinkType, std::function()> sinkFactory, logging::Severity severity); /** Change the minimum severity level for log messages to be output by the default logger. diff --git a/include/onnxruntime/core/common/logging/sink_types.h b/include/onnxruntime/core/common/logging/sink_types.h new file mode 100644 index 0000000000000..a99b0fca58d9d --- /dev/null +++ b/include/onnxruntime/core/common/logging/sink_types.h @@ -0,0 +1,11 @@ +#pragma once + +namespace onnxruntime { +namespace logging { +enum class SinkType { + BaseSink, + CompositeSink, + EtwSink +}; +} // namespace logging +} // namespace onnxruntime diff --git a/onnxruntime/core/common/logging/logging.cc b/onnxruntime/core/common/logging/logging.cc index e9b61e80f46a1..eb171e0953fd6 100644 --- a/onnxruntime/core/common/logging/logging.cc +++ b/onnxruntime/core/common/logging/logging.cc @@ -281,46 +281,39 @@ Severity OverrideLevelWithEtw(Severity originalSeverity) { return originalSeverity; } -#ifdef _WIN32 -void LoggingManager::AddEtwSink(logging::Severity etwSeverity) { +void LoggingManager::AddSink(SinkType sinkType, std::function()> sinkFactory, logging::Severity severity) { std::lock_guard guard(sink_mutex_); - // Check if the EtwRegistrationManager is enabled - auto& manager = EtwRegistrationManager::Instance(); - if (!manager.IsEnabled()) { - return; // ETW not enabled, no operation needed - } - - if (sink_->GetType() != ISink::CompositeSink) { + if (sink_->GetType() != SinkType::CompositeSink) { // Current sink is not a composite, create a new composite sink and add the current sink to it auto new_composite = std::make_unique(); new_composite->AddSink(std::move(sink_), default_min_severity_); // Move the current sink into the new composite sink_ = std::move(new_composite); // Now sink_ is pointing to the new composite } - // Adjust the default minimum severity level to accommodate ETW logging needs - default_min_severity_ = std::min(default_min_severity_, etwSeverity); + // Adjust the default minimum severity level to accommodate new sink needs + default_min_severity_ = std::min(default_min_severity_, severity); if (s_default_logger_ != nullptr) { s_default_logger_->SetSeverity(default_min_severity_); } CompositeSink* current_composite = static_cast(sink_.get()); - // Check if an EtwSink already exists in the current composite const auto& sinks = current_composite->GetSinks(); - if (std::any_of(sinks.begin(), sinks.end(), [](const auto& pair) { - return pair.first->GetType() == ISink::EtwSink; + + // Check if a sink of this type already exists + if (std::any_of(sinks.begin(), sinks.end(), [sinkType](const auto& pair) { + return pair.first->GetType() == sinkType; })) { - return; // EtwSink already exists, do not add another + return; // Sink of this type already exists, do not add another } - // Add a new EtwSink - current_composite->AddSink(std::make_unique(), etwSeverity); + current_composite->AddSink(sinkFactory(), severity); } -void LoggingManager::RemoveEtwSink() { +void LoggingManager::RemoveSink(SinkType sinkType) { std::lock_guard guard(sink_mutex_); - if (sink_->GetType() == ISink::CompositeSink) { + if (sink_->GetType() == SinkType::CompositeSink) { auto composite_sink = static_cast(sink_.get()); const auto& sinks_with_severity = composite_sink->GetSinks(); std::unique_ptr remaining_sink; @@ -328,9 +321,9 @@ void LoggingManager::RemoveEtwSink() { Severity newSeverity = Severity::kFATAL; for (const auto& sink_pair : sinks_with_severity) { - if (sink_pair.first->GetType() != ISink::EtwSink) { + if (sink_pair.first->GetType() != sinkType) { if (remaining_sink) { - // If more than one non-EtwSink is found, we leave the CompositeSink intact + // If more than one non sinkType is found, we leave the CompositeSink intact return; } newSeverity = std::min(newSeverity, sink_pair.second); @@ -338,13 +331,12 @@ void LoggingManager::RemoveEtwSink() { } } - // If only one non-EtwSink remains, replace the CompositeSink with this sink + // If only one non sinkType remains, replace the CompositeSink with this sink if (remaining_sink) { sink_ = std::move(remaining_sink); } else { - // Handle the case where all sinks were EtwSinks - // sink_ = std::make_unique(); // Assuming NullSink is a basic ISink that does nothing + // Handle the case where all sinks were sinkType } default_min_severity_ = newSeverity; @@ -353,7 +345,6 @@ void LoggingManager::RemoveEtwSink() { } } } -#endif } // namespace logging } // namespace onnxruntime diff --git a/onnxruntime/core/common/logging/sinks/composite_sink.h b/onnxruntime/core/common/logging/sinks/composite_sink.h index 832e0327d23b9..fd613f1fab98e 100644 --- a/onnxruntime/core/common/logging/sinks/composite_sink.h +++ b/onnxruntime/core/common/logging/sinks/composite_sink.h @@ -23,9 +23,7 @@ class CompositeSink : public ISink { /// Initializes a new instance of the class. /// Use AddSink to add sinks. /// - CompositeSink() {} - - SinkType GetType() const override { return ISink::CompositeSink; } + CompositeSink() : ISink(SinkType::CompositeSink) {} /// /// Adds a sink. Takes ownership of the sink (so pass unique_ptr by value). diff --git a/onnxruntime/core/platform/windows/logging/etw_sink.h b/onnxruntime/core/platform/windows/logging/etw_sink.h index 4efa6ed4e32df..5d35d101f1242 100644 --- a/onnxruntime/core/platform/windows/logging/etw_sink.h +++ b/onnxruntime/core/platform/windows/logging/etw_sink.h @@ -31,11 +31,9 @@ namespace logging { class EtwSink : public ISink { public: - EtwSink() = default; + EtwSink() : ISink(SinkType::EtwSink) {} ~EtwSink() = default; - SinkType GetType() const override { return ISink::EtwSink; } - constexpr static const char* kEventName = "ONNXRuntimeLogEvent"; private: diff --git a/onnxruntime/core/session/inference_session.cc b/onnxruntime/core/session/inference_session.cc index f801929557d51..3386a6ed15d58 100644 --- a/onnxruntime/core/session/inference_session.cc +++ b/onnxruntime/core/session/inference_session.cc @@ -12,6 +12,7 @@ #include #include "core/common/denormal.h" +#include "core/common/logging/isink.h" #include "core/common/logging/logging.h" #include "core/common/parse_string.h" #include "core/common/path_string.h" @@ -52,6 +53,7 @@ #include "core/platform/tracing.h" #include #include "core/platform/windows/telemetry.h" +#include "core/platform/windows/logging/etw_sink.h" #endif #include "core/providers/cpu/controlflow/utils.h" #include "core/providers/cpu/cpu_execution_provider.h" @@ -420,13 +422,19 @@ void InferenceSession::ConstructorCommon(const SessionOptions& session_options, if ((MatchAnyKeyword & static_cast(onnxruntime::logging::ORTTraceLoggingKeyword::Logs)) != 0 && IsEnabled == EVENT_CONTROL_CODE_ENABLE_PROVIDER) { LOGS(*session_logger_, VERBOSE) << "Adding ETW Sink to logger with severity level: " << (ULONG)ortETWSeverity; - logging_manager_->AddEtwSink(ortETWSeverity); - onnxruntime::logging::LoggingManager::GetDefaultInstance()->AddEtwSink(ortETWSeverity); + logging_manager_->AddSink( + onnxruntime::logging::SinkType::EtwSink, + []() -> std::unique_ptr { return std::make_unique(); }, + ortETWSeverity); + onnxruntime::logging::LoggingManager::GetDefaultInstance()->AddSink( + onnxruntime::logging::SinkType::EtwSink, + []() -> std::unique_ptr { return std::make_unique(); }, + ortETWSeverity); LOGS(*session_logger_, INFO) << "Done Adding ETW Sink to logger with severity level: " << (ULONG)ortETWSeverity; } if (IsEnabled == EVENT_CONTROL_CODE_DISABLE_PROVIDER) { LOGS(*session_logger_, INFO) << "Removing ETW Sink from logger"; - logging_manager_->RemoveEtwSink(); + logging_manager_->RemoveSink(onnxruntime::logging::SinkType::EtwSink); LOGS(*session_logger_, VERBOSE) << "Done Removing ETW Sink from logger"; } }