-
Notifications
You must be signed in to change notification settings - Fork 117
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
Multi-process shared files #14
Conversation
…needs some refactoring...
#endif | ||
|
||
if (buffered) | ||
throw new ArgumentException("Buffered writes are not available when file sharing is enabled."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add nameof(buffered)
as argument here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
LGTM |
if (_countingStreamWrapper.CountedLength >= _fileSizeLimitBytes.Value) | ||
return; | ||
} | ||
|
||
_textFormatter.Format(logEvent, _output); | ||
if (!_buffered) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any concerns about moving if (...) _output.Flush() ouf of the lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, neither the file nor writer is thread-safe. There's also a delicate balancing act going on: each write has to begin with all internal buffers flushed so that the next event is written into the FileStream's buffer starting precisely at position zero, so that the following Flush() call causes a single write through to the underlying Win32 native function. If we don't do this, and parts of events get written via different Win32 calls, the writes may be interleaved with writes from other processes.
Depends on #13, which needs to be merged first.
This PR adds a
shared
parameter toWriteTo.File()
:When
shared
is set:, the file will be opened withFileShare.Write
andFILE_APPEND_DATA
. Events will be buffered into a singlebyte[]
and each event will result in a singleFileStream.Write()
call.This causes Windows to synchronize writes from multiple processes so that log messages are not interleaved.
The cost of the feature is the additional buffering, required because
FileStream
's API does not support this style of use well. In profiling the added overhead is ~5% of total run time, so not a big deal.The two buffers used will grow to the size of the largest logged event, so there may be some additional memory pressure if large events come through.
There's room for optimisation, but, surprisingly,
FILE_APPEND_DATA
improves performance of even non-shared writes by about 30%. For that reason, the PR enables it when available in the non-shared case as well, dropping the time to write 1M events with the included app from 13400 ms to 10600 ms.The feature is only available on .NET 4.x, as the flags can't yet be set (without perhaps PInvoke) on .NET Core.
Breaks binary compatibility with
WriteTo.File()
, but does not modify theFileSink
constructor, so downstream sinks (likeRollingFile
) are unlikely to be affected. A big enough feature addition to warrant a major version bump however.Closes #7.