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.

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

ColoredConsoleTarget performance improvements. #1112

Merged
merged 8 commits into from Jan 8, 2016

Conversation

bhaeussermann
Copy link
Contributor

Fixes #980

I've identified the expensive operations to be the ForegroundColor and BackgroundColor properties (both get and set), and the TextWriter.Write() and TextWriter.WriteLine() methods.

I've applied two performance enhancements:

  1. When there are no word highlighting rules, then we WriteLine(message) the message instead of doing a Write(message) followed by a WriteLine(). This saves one expensive Write() call per log.
  2. We set the colors only if they must change (i.e. the matching rule's color is different).

To test the performance I built a program that logs 2000 messages and every 13th message is done with a different log level causing that message to appear in a different color. All other messages appear in the default console colors.

Before applying any of the improvements, the logging took 0.8 seconds on average. The ConsoleTarget takes about 0.25 seconds to log the same messages.

Change (1) yields an improvement of about 38 %, and change (2) yields an improvement of about 46 %.
Together, the two changes decrease the running time from 0.8 seconds to about 0.33 seconds; a total improvement of about 59 %. It is also quite close to the ConsoleTarget time of 0.25 seconds.

@codecov-io
Copy link

Current coverage is 73.35%

Merging #1112 into master will increase coverage by +0.03% as of 9af56f7

@@            master   #1112   diff @@
======================================
  Files          264     264       
  Stmts        14944   14945     +1
  Branches      1636    1638     +2
  Methods          0       0       
======================================
+ Hit          10958   10963     +5
- Partial        417     418     +1
+ Missed        3569    3564     -5

Review entire Coverage Diff as of 9af56f7


Uncovered Suggestions

  1. +0.10% via ...nternal/AspHelper.cs#206...218
  2. +0.10% via ...nternal/AspHelper.cs#188...200
  3. +0.10% via ...nternal/AspHelper.cs#170...182
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

/// Gets the compiled regular expression that matches either Text or Regex property.
/// </summary>
public Regex CompiledRegex
private Regex CompiledRegex
Copy link
Member

Choose a reason for hiding this comment

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

This is an unneeded breaking change?

@bhaeussermann
Copy link
Contributor Author

I reverted the breaking change in ConsoleWordHighlightingRule. Thanks!

{
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.

@304NotModified
Copy link
Member

Hi, Thanks for this PR!

Just reviewed the code. two notes. Also, I think I think it would be nice to remove the regex fully when it's just a string replace, although I can be hard to keep that backwards-compatible. What do you think?

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 4, 2016
@bhaeussermann
Copy link
Contributor Author

I think I think it would be nice to remove the regex fully when it's just a string replace, although I can be hard to keep that backwards-compatible. What do you think?

I tested this and it yields no significant difference in performance, so it's probably not worth adding the additional complexity to the code.

@bhaeussermann
Copy link
Contributor Author

Xamarin license issue again :(

@304NotModified
Copy link
Member

My local machine seems to incorrectly unload the licenses. :(

@304NotModified
Copy link
Member

fixed. The deadlocked machines took some Xamarin licenses as my PC (deactivated now)

{
var matchingRule = GetMatchingRowHighlightingRule(logEvent);

if (ColorDoesChange(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.

I think this is easier to read:

var didChangeForegroundColor  = ColorDoesChange(matchingRule.ForegroundColor, oldForegroundColor)`
if(didChangeForegroundColor  ){}

what do you think?

@bhaeussermann
Copy link
Contributor Author

Thanks for the good proposals! I agree and I applied them.

@304NotModified
Copy link
Member

Great! Thanks! 👍

304NotModified added a commit that referenced this pull request Jan 8, 2016
ColoredConsoleTarget performance improvements.
@304NotModified 304NotModified merged commit e92ef35 into NLog:master Jan 8, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Jan 8, 2016
@snakefoot snakefoot added performance and removed enhancement Improvement on existing feature labels Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants