Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 12, 2025

User description

💥 What does this PR do?

This pull request optimizes the logging methods in the Logger class for better performance by applying aggressive inlining. This can help reduce the overhead of method calls, especially for frequently used logging operations.

🔧 Implementation Notes

Performance improvements:

  • Added [MethodImpl(MethodImplOptions.AggressiveInlining)] to all public logging methods (Trace, Debug, Info, Warn, Error) and internal methods (IsEnabled, LogMessage) in Logger.cs to encourage the compiler to inline these methods for faster execution.
  • Imported System.Runtime.CompilerServices in Logger.cs to support the use of the MethodImpl attribute.

PR Type

Enhancement


Description

  • Applied aggressive inlining to all public logging methods (Trace, Debug, Info, Warn, Error)

  • Added aggressive inlining to internal helper methods (IsEnabled, LogMessage)

  • Imported System.Runtime.CompilerServices namespace for MethodImpl attribute support


Diagram Walkthrough

flowchart LR
  A["Logger.cs"] --> B["Add MethodImpl attribute"]
  B --> C["Public methods: Trace, Debug, Info, Warn, Error"]
  B --> D["Internal methods: IsEnabled, LogMessage"]
  C --> E["Performance optimization"]
  D --> E
Loading

File Walkthrough

Relevant files
Performance
Logger.cs
Apply aggressive inlining to Logger methods                           

dotnet/src/webdriver/Internal/Logging/Logger.cs

  • Added [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute to
    all public logging methods
  • Applied aggressive inlining to internal IsEnabled and LogMessage
    methods
  • Imported System.Runtime.CompilerServices namespace
+8/-0     

Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid unnecessary log message construction

Add a check for the log level using IsEnabled(level) at the beginning of each
logging method (Trace, Debug, etc.) to avoid unnecessary work when the level is
disabled.

dotnet/src/webdriver/Internal/Logging/Logger.cs [41-69]

 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Trace(string message)
 {
-    LogMessage(LogEventLevel.Trace, message);
+    if (IsEnabled(LogEventLevel.Trace))
+    {
+        LogMessage(LogEventLevel.Trace, message);
+    }
 }
 
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Debug(string message)
 {
-    LogMessage(LogEventLevel.Debug, message);
+    if (IsEnabled(LogEventLevel.Debug))
+    {
+        LogMessage(LogEventLevel.Debug, message);
+    }
 }
 
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Info(string message)
 {
-    LogMessage(LogEventLevel.Info, message);
+    if (IsEnabled(LogEventLevel.Info))
+    {
+        LogMessage(LogEventLevel.Info, message);
+    }
 }
 
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Warn(string message)
 {
-    LogMessage(LogEventLevel.Warn, message);
+    if (IsEnabled(LogEventLevel.Warn))
+    {
+        LogMessage(LogEventLevel.Warn, message);
+    }
 }
 
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Error(string message)
 {
-    LogMessage(LogEventLevel.Error, message);
+    if (IsEnabled(LogEventLevel.Error))
+    {
+        LogMessage(LogEventLevel.Error, message);
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly proposes a standard optimization for logging frameworks by checking if a log level is enabled before processing the message, which aligns with the PR's performance-oriented changes.

Low
Learned
best practice
Avoid blanket aggressive inlining

Remove AggressiveInlining from public methods and keep it only where proven
beneficial via profiling; prefer leaving the JIT to decide for tiny wrappers
unless there's a demonstrated hot path.

dotnet/src/webdriver/Internal/Logging/Logger.cs [41-81]

-[MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Trace(string message)
 {
     LogMessage(LogEventLevel.Trace, message);
 }
 
-[MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Debug(string message)
 {
     LogMessage(LogEventLevel.Debug, message);
 }
 
-[MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Info(string message)
 {
     LogMessage(LogEventLevel.Info, message);
 }
 
-[MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Warn(string message)
 {
     LogMessage(LogEventLevel.Warn, message);
 }
 
-[MethodImpl(MethodImplOptions.AggressiveInlining)]
 public void Error(string message)
 {
     LogMessage(LogEventLevel.Error, message);
 }
 
-[MethodImpl(MethodImplOptions.AggressiveInlining)]
 public bool IsEnabled(LogEventLevel level)
 {
     return Log.CurrentContext.IsEnabled(this, level);
 }
 
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 private void LogMessage(LogEventLevel level, string message)
 {
     Log.CurrentContext.EmitMessage(this, level, message);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Keep API and documentation accurate and consistent by aligning public surface with appropriate performance annotations; avoid overusing AggressiveInlining, which is a hint and can harm maintainability and JIT decisions.

Low
  • More

Copy link
Contributor

@RenderMichael RenderMichael left a comment

Choose a reason for hiding this comment

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

Do we have performance numbers justifying this change?

@nvborisenko
Copy link
Member Author

1 nanosecond? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants