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

FilePathLayout - Reduce memory-allocation for cleanup of filename #1745

Merged
merged 1 commit into from
Nov 7, 2016

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 6, 2016

Reuse clean-filename when raw-filename is unchanged.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Nov 6, 2016

Current coverage is 80% (diff: 96%)

Merging #1745 into master will increase coverage by <1%

@@             master      #1745   diff @@
==========================================
  Files           274        274          
  Lines         17066      17079    +13   
  Methods        2698       2699     +1   
  Messages          0          0          
  Branches       1910       1912     +2   
==========================================
+ Hits          13686      13711    +25   
- Misses         2931       2932     +1   
+ Partials        449        436    -13   

Sunburst

Powered by Codecov. Last update 35259db...acc8071

@snakefoot snakefoot changed the title FilePathLayout - Reduce string-allocation for cleanup of filename FilePathLayout - Reduce memory-allocation for cleanup of filename Nov 6, 2016
@snakefoot snakefoot force-pushed the FilePathLayoutCachePrev branch 2 times, most recently from 9f307d9 to 2a4aa22 Compare November 6, 2016 18:40
@304NotModified 304NotModified added this to the 4.3.11 milestone Nov 6, 2016
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

Added two small notes

@@ -77,6 +77,9 @@ internal class FilePathLayout : IRenderable

private bool _cleanupInvalidChars;

private string _cachedPrevRawFileName;
private string _cachedPrevCleanFileName;
Copy link
Member

Choose a reason for hiding this comment

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

We should document (xmldoc) that _cachedPrevCleanFileName is a pair with _cachedPrevRawFileName, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they are working together. Will update the xml-doc

if ((!_cleanupInvalidChars || cleanedFixedResult != null) && _filePathKind == FilePathKind.Absolute)
return rawFileName; // Skip clean filename string-allocation

if (string.CompareOrdinal(_cachedPrevRawFileName, rawFileName) == 0 && _cachedPrevCleanFileName != null)
Copy link
Member

Choose a reason for hiding this comment

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

In case of performance, the check _cachedPrevCleanFileName != null should be on the left?

Copy link
Contributor Author

@snakefoot snakefoot Nov 6, 2016

Choose a reason for hiding this comment

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

_cachedPrevCleanFileName will never be null, and when it is then _cachedPrevRawFileName is also null. I just have a secret agenda when it comes to the PR with PoolSetup :)

…le (Reuse clean-filename when raw-filename is unchanged)
@304NotModified
Copy link
Member

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@304NotModified 304NotModified merged commit 64fb715 into NLog:master Nov 7, 2016
@304NotModified
Copy link
Member

Thanks again!

@snakefoot snakefoot deleted the FilePathLayoutCachePrev branch October 10, 2017 20:40
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

3 participants