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

ColoredConsoleTarget performance improvements. #1112

Merged
merged 8 commits into from Jan 8, 2016
Merged

ColoredConsoleTarget performance improvements. #1112

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4b88a21
ConsoleWordHighlightingRule: Don't use compiled regexes.
bhaeussermann Dec 26, 2015
79375af
ColoredConsoleTarget: Performance improvement.
bhaeussermann Dec 26, 2015
e2d3a17
Refactor ColoredConsoleTarget Output() method.
bhaeussermann Dec 26, 2015
a564528
ConsoleWordHighlightingRule: Don't use compiled regexes.
bhaeussermann Jan 1, 2016
6b89e23
Refactor ColoredConsoleTarget.
bhaeussermann Jan 5, 2016
562eb6b
Merge branch 'master' of https://github.com/NLog/NLog
bhaeussermann Jan 7, 2016
756f2a8
ConsoleWordHighlightingRule: Use compiled regex.
bhaeussermann Jan 8, 2016
f8e4a6d
Refactor ColoredConsoleTarget.
bhaeussermann Jan 8, 2016
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
140 changes: 70 additions & 70 deletions src/NLog/Targets/ColoredConsoleTarget.cs
View file
Open in desktop
Expand Up @@ -187,6 +187,76 @@ protected override void Write(LogEventInfo logEvent)
this.Output(logEvent, this.Layout.Render(logEvent));
}

private void Output(LogEventInfo logEvent, string message)
{
ConsoleColor oldForegroundColor = Console.ForegroundColor;
ConsoleColor oldBackgroundColor = Console.BackgroundColor;
bool didChangeForegroundColor = false, didChangeBackgroundColor = false;

try
{
var matchingRule = GetMatchingRowHighlightingRule(logEvent);

if ((matchingRule.ForegroundColor != ConsoleOutputColor.NoChange) && ((ConsoleColor)matchingRule.ForegroundColor != oldForegroundColor))
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract this to a method? (DRY)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
Console.ForegroundColor = (ConsoleColor)matchingRule.ForegroundColor;
didChangeForegroundColor = true;
}

if ((matchingRule.BackgroundColor != ConsoleOutputColor.NoChange) && ((ConsoleColor)matchingRule.BackgroundColor != oldBackgroundColor))
{
Console.BackgroundColor = (ConsoleColor)matchingRule.BackgroundColor;
didChangeBackgroundColor = true;
}

var consoleStream = this.ErrorStream ? Console.Error : Console.Out;
if (this.WordHighlightingRules.Count == 0)
{
consoleStream.WriteLine(message);
}
else
{
message = message.Replace("\a", "\a\a");
foreach (ConsoleWordHighlightingRule hl in this.WordHighlightingRules)
{
message = hl.ReplaceWithEscapeSequences(message);
}

ColorizeEscapeSequences(consoleStream, message, new ColorPair(Console.ForegroundColor, Console.BackgroundColor), new ColorPair(oldForegroundColor, oldBackgroundColor));
consoleStream.WriteLine();

didChangeForegroundColor = didChangeBackgroundColor = true;
}
}
finally
{
if (didChangeForegroundColor)
Console.ForegroundColor = oldForegroundColor;
if (didChangeBackgroundColor)
Console.BackgroundColor = oldBackgroundColor;
}
}

private ConsoleRowHighlightingRule GetMatchingRowHighlightingRule(LogEventInfo logEvent)
{
foreach (ConsoleRowHighlightingRule rule in this.RowHighlightingRules)
{
if (rule.CheckCondition(logEvent))
return rule;
}

if (this.UseDefaultRowHighlightingRules)
{
foreach (ConsoleRowHighlightingRule rule in defaultConsoleRowHighlightingRules)
{
if (rule.CheckCondition(logEvent))
return rule;
}
}

return ConsoleRowHighlightingRule.Default;
}

private static void ColorizeEscapeSequences(
TextWriter output,
string message,
Expand Down Expand Up @@ -285,76 +355,6 @@ protected override void Write(LogEventInfo logEvent)
}
}

private void Output(LogEventInfo logEvent, string message)
{
ConsoleColor oldForegroundColor = Console.ForegroundColor;
ConsoleColor oldBackgroundColor = Console.BackgroundColor;

try
{
ConsoleRowHighlightingRule matchingRule = null;

foreach (ConsoleRowHighlightingRule cr in this.RowHighlightingRules)
{
if (cr.CheckCondition(logEvent))
{
matchingRule = cr;
break;
}
}

if (this.UseDefaultRowHighlightingRules && matchingRule == null)
{
foreach (ConsoleRowHighlightingRule cr in defaultConsoleRowHighlightingRules)
{
if (cr.CheckCondition(logEvent))
{
matchingRule = cr;
break;
}
}
}

if (matchingRule == null)
{
matchingRule = ConsoleRowHighlightingRule.Default;
}

if (matchingRule.ForegroundColor != ConsoleOutputColor.NoChange)
{
Console.ForegroundColor = (ConsoleColor)matchingRule.ForegroundColor;
}

if (matchingRule.BackgroundColor != ConsoleOutputColor.NoChange)
{
Console.BackgroundColor = (ConsoleColor)matchingRule.BackgroundColor;
}

message = message.Replace("\a", "\a\a");

foreach (ConsoleWordHighlightingRule hl in this.WordHighlightingRules)
{
message = hl.ReplaceWithEscapeSequences(message);
}

ColorizeEscapeSequences(this.ErrorStream ? Console.Error : Console.Out, message, new ColorPair(Console.ForegroundColor, Console.BackgroundColor), new ColorPair(oldForegroundColor, oldBackgroundColor));
}
finally
{
Console.ForegroundColor = oldForegroundColor;
Console.BackgroundColor = oldBackgroundColor;
}

if (this.ErrorStream)
{
Console.Error.WriteLine();
}
else
{
Console.WriteLine();
}
}

/// <summary>
/// Color pair (foreground and background).
/// </summary>
Expand Down
32 changes: 16 additions & 16 deletions src/NLog/Targets/ConsoleWordHighlightingRule.cs
View file
Open in desktop
Expand Up @@ -96,6 +96,20 @@ public ConsoleWordHighlightingRule(string text, ConsoleOutputColor foregroundCol
[DefaultValue(false)]
public bool IgnoreCase { get; set; }

/// <summary>
/// Gets or sets the foreground color.
/// </summary>
/// <docgen category='Formatting Options' order='10' />
[DefaultValue("NoChange")]
public ConsoleOutputColor ForegroundColor { get; set; }

/// <summary>
/// Gets or sets the background color.
/// </summary>
/// <docgen category='Formatting Options' order='10' />
[DefaultValue("NoChange")]
public ConsoleOutputColor BackgroundColor { get; set; }

/// <summary>
/// Gets the compiled regular expression that matches either Text or Regex property.
/// </summary>
Expand All @@ -116,7 +130,7 @@ public Regex CompiledRegex
}
}

RegexOptions regexOptions = RegexOptions.Compiled;
RegexOptions regexOptions = RegexOptions.None;
Copy link
Member

Choose a reason for hiding this comment

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

Is this a performance boost according to the test?

Because we also won't use regex caching, (static regex), which is maybe not that nice.

Also I'm bit in doubt with this change due to the name of the property. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change does not yield a noticeable change in performance. The reason I changed this was due to a previous comment referring to the MSDN documentation stating that the Compiled option compiles the regular expression to MSIL code in an assembly which cannot be unloaded. I believe the concern was that this might cause increased memory usage in the long run. However, coming to think of it, if the regular expression does not change (which will typically be the case), this should not be an issue.

I personally do not think this makes much of a difference. I'll change it back if you want.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should pick one of the two: use caching, or use compied. As this one isn't 1 of them, I prefer a revert (also because of the name).

if (this.IgnoreCase)
{
regexOptions |= RegexOptions.IgnoreCase;
Expand All @@ -129,21 +143,7 @@ public Regex CompiledRegex
}
}

/// <summary>
/// Gets or sets the foreground color.
/// </summary>
/// <docgen category='Formatting Options' order='10' />
[DefaultValue("NoChange")]
public ConsoleOutputColor ForegroundColor { get; set; }

/// <summary>
/// Gets or sets the background color.
/// </summary>
/// <docgen category='Formatting Options' order='10' />
[DefaultValue("NoChange")]
public ConsoleOutputColor BackgroundColor { get; set; }

internal string MatchEvaluator(Match m)
private string MatchEvaluator(Match m)
{
StringBuilder result = new StringBuilder();

Expand Down