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

FileTarget - RetryingMultiProcessFileAppender should use BufferSize=1024 #1864

Closed
wants to merge 1 commit into from

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Dec 18, 2016

Logger Name Messages Size Threads Loggers
PerfSyncLogger 10.000 16 1 1
BufferSize Time (ms) Msgs/sec GC2 GC1 GC0 CPU (ms) Mem (MB)
32768 26.918 371 0 1 22 3.312 36,332
4096 26.745 373 0 0 3 3.203 35,680
1024 26.433 378 0 0 1 3.093 35,496

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Dec 18, 2016

Current coverage is 82% (diff: 100%)

Merging #1864 into master will decrease coverage by <1%

@@             master      #1864   diff @@
==========================================
  Files           276        276          
  Lines         18472      18471     -1   
  Methods        2857       2856     -1   
  Messages          0          0          
  Branches       2129       2130     +1   
==========================================
- Hits          15061      15055     -6   
- Misses         2922       2928     +6   
+ Partials        489        488     -1   

Sunburst

Powered by Codecov. Last update fa1eb2d...00324e9

@304NotModified
Copy link
Member

Less then 2% improvement. Not sure if this benefit is large enough. What do you think?

Reason not the merge:

  • possible slower in other configurations
  • buffersize is "dynamic", more difficult to document

@snakefoot
Copy link
Contributor Author

Tried some testing with the FileTarget within the AsyncWrapper, and the improvement was also small there. Will close this.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 8, 2017

@tlaguz Regarding " Unhandled exception when writing to file". Consider changing your NLog-File-Target to use keepFileOpen="true" or specify bufferSize="1024". This will avoid the allocation of 30 KByte buffer for every log-event-write-operation (No longer needed with #3444 with NLog 4.6.5)

@snakefoot snakefoot deleted the FileTargetBufferSize branch October 10, 2017 20:39
@snakefoot
Copy link
Contributor Author

Superseeded by #3444

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