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

WindowsMultiProcessFileAppender - FileSystemRights.AppendData #1756

Merged

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 10, 2016

Merged #1474 with master, and changed from native windows file handle to managed FileStream-object

Using FileSystemRights.AppendData (FILE_APPEND_DATA / O_APPEND) allows multiple processes to write to the same local file without needing a global mutex object. This improves file-write performance with a factor 2.

There are different rumors on the Internet whether FILE_APPEND_DATA works correctly when the number of bytes written in a single operation is larger than 1KByte or 4KByte. The file-size increase that allocates the needed room in the file is atomic. But writing the bytes to the allocated room is probably done in sector size depending on version of operating system. This means if having very aggressive tail application that performs direct file reads, then it can momentarily read the allocated room without data has actually been written.


This change is Reviewable

@snakefoot snakefoot force-pushed the andre-unleased-fileappender-v2 branch 3 times, most recently from 0144b18 to 7caaaf9 Compare November 10, 2016 20:41
@snakefoot snakefoot changed the title Andre unleased fileappender v2 WindowsMultiProcessFileAppender - FileSystemRights.AppendData Nov 10, 2016
@snakefoot snakefoot force-pushed the andre-unleased-fileappender-v2 branch from 7caaaf9 to 1bf7eaa Compare November 10, 2016 20:47
@snakefoot
Copy link
Contributor Author

I have tested the updated solution with the test-project from #1474 made by @AndreGleichner, and it completes and performs successful verification.

@codecov-io
Copy link

codecov-io commented Nov 10, 2016

Current coverage is 81% (diff: 82%)

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

@@             master      #1756   diff @@
==========================================
  Files           276        277     +1   
  Lines         17523      17626   +103   
  Methods        2756       2767    +11   
  Messages          0          0          
  Branches       1975       1989    +14   
==========================================
+ Hits          14089      14226   +137   
+ Misses         2988       2947    -41   
- Partials        446        453     +7   

Sunburst

Powered by Codecov. Last update cd62cd0...80e40da

@304NotModified
Copy link
Member

There are different rumors on the Internet whether FILE_APPEND_DAT

So it's untrue? Or unsure?

@304NotModified 304NotModified added this to the 4.4 milestone Nov 13, 2016
@304NotModified 304NotModified added performance enhancement Improvement on existing feature feature labels Nov 13, 2016
@304NotModified
Copy link
Member

Thanks for the changes! Will try to review it soon and release in into 4.4 (rc)

@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 16, 2016

Have noticed that the unit test time has increased. Most likely because of the added combinations in FileTargetTests.cs (Linq-booleanValues).

Not sure if this is a good direction, as every new parameter will double the time.

@304NotModified
Copy link
Member

Good to know. It was ~11min and now ~19.

In my experience 11 was already to long. When working on several items, I have to wait for hours.

Everything is tested with net35+net40+net45. Maybe we should test some tests only with net45

@snakefoot snakefoot force-pushed the andre-unleased-fileappender-v2 branch 2 times, most recently from 05f3f0c to 15647f6 Compare November 17, 2016 00:22
@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 17, 2016

Added a filter so it only exercises the major FileAppenders-combinations. Though it just removed 1-2 minutes.

Travis Test Count = 1770 (Before=2190), but still an increase from current master=1554
AppVeyor Test Count = 1915 (Before=2355), but still an increase from current master=1677

It seems the time is spend by the code-coverage, as the other builds completes quickly, so not sure if the skipping the FileTargetTest for the other builds will help that much.

@snakefoot snakefoot force-pushed the andre-unleased-fileappender-v2 branch 5 times, most recently from 44b0297 to a1f1330 Compare November 19, 2016 15:48
@snakefoot snakefoot force-pushed the andre-unleased-fileappender-v2 branch from a1f1330 to 80e40da Compare November 19, 2016 16:11
@304NotModified
Copy link
Member

Will try to test this next week

@304NotModified 304NotModified self-assigned this Nov 20, 2016
@304NotModified
Copy link
Member

Reviewed 1 of 20 files at r1, 23 of 24 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/NLog/Internal/FileCharacteristicsHelper.cs, line 68 at r2 (raw file):

        /// <param name="fileStream">The file stream.</param>
        /// <returns>The file characteristics, if the file information was retrieved successfully, otherwise null.</returns>
        public abstract FileCharacteristics GetFileCharacteristics(string fileName, FileStream fileStream);

Is this not infecting performance?


Comments from Reviewable

@304NotModified
Copy link
Member

304NotModified commented Nov 23, 2016

Need docs for

  • ForceManaged ,
  • ForceMutexConcurrentWrites

@snakefoot
Copy link
Contributor Author

src/NLog/Internal/FileCharacteristicsHelper.cs, line 68 at r2 (raw file):

Previously, 304NotModified (Julian Verdurmen) wrote… > Is this not infecting performance?
Changing the parameter from IntPtr to FileStream-reference ?

Comments from Reviewable

@304NotModified
Copy link
Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/NLog/Internal/FileCharacteristicsHelper.cs, line 68 at r2 (raw file):

Previously, snakefoot (Rolf Kristensen) wrote… > Changing the parameter from IntPtr to FileStream-reference ?
Well using FileStream insteaf of IntPtr

Comments from Reviewable

@snakefoot
Copy link
Contributor Author

src/NLog/Internal/FileCharacteristicsHelper.cs, line 68 at r2 (raw file):

Previously, 304NotModified (Julian Verdurmen) wrote… > Well using FileStream insteaf of IntPtr
I'm only using the FileStream-parameter to carry the IntPtr (If needed). If using Win32FileCharacteristicsHelper then it will use fileStream.SafeFileHandle.DangerousGetHandle()

Comments from Reviewable

@304NotModified
Copy link
Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/NLog/Internal/FileCharacteristicsHelper.cs, line 68 at r2 (raw file):

Previously, snakefoot (Rolf Kristensen) wrote… > I'm only using the FileStream-parameter to carry the IntPtr (If needed). If using Win32FileCharacteristicsHelper then it will use fileStream.SafeFileHandle.DangerousGetHandle()
OK thanks! :)

Comments from Reviewable

@304NotModified 304NotModified changed the base branch from master to release/4.4 November 25, 2016 23:54
@304NotModified 304NotModified merged commit f9473d5 into NLog:release/4.4 Nov 25, 2016
@304NotModified
Copy link
Member

from tests give me ~10% performance gain, also with 1 thread.

tested with

NLogPerformance\bin\Release\NLogPerformance.exe 3000000 1  8  1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 1  16 1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 1  32 1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 2  16 1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 4  16 1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 8  16 1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 16 16 1 false >> performance-log.log
NLogPerformance\bin\Release\NLogPerformance.exe 3000000 60 16 1 false >> performance-log.log

@304NotModified
Copy link
Member

Will be in news post

@snakefoot snakefoot deleted the andre-unleased-fileappender-v2 branch October 10, 2017 20:40
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.

None yet

4 participants