-
Notifications
You must be signed in to change notification settings - Fork 46
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
Reduce allocations during message creation #200
Conversation
86c190f
to
f26a650
Compare
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.
Please take a look at the comments and change your code
src/NLog.Targets.Syslog/ByteArray.cs
Outdated
return; | ||
|
||
var byteCount = encoding.GetByteCount(buffer); | ||
memoryStream.SetLength(memoryStream.Length + byteCount); |
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.
Memory stream is not meant to be resized in the Syslog target: it is used as a buffer to avoid allocations and it has to keep the size of the intial capacity as computed by EnforceAllowedValues at construction time
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.
This Append-method has the exact same behavior as the Append(byte[])
-method, but without memory-allocation. When calling MemoryStream.Write(byte[])
then one advances MemoryStream.Length
and MemoryStream.Position
. MemoryStream.SetLength()
just changes the Length-property, and only re-allocates if Capacity is too small. So I don't understand the issue about resize.
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.
The initialCapacity
of the ByteArray
is actually a number of bytes even if the number of character is passed: the underlying memory stream will handle growth automatically during write.
Anyway an issue an PR should be opened to handle the case when the capacity is MaxBufferCapacity
and we are trying to write a string longer than the remaining capacity.
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.
When someone tries to write a payload of int.MaxValue bytes, then other obstacles will probably prevent it from ever happening. But one can easily imagine writing a buffer larger than initialCapacity, and in that case then MemoryStream.Write(byte[]) will grow the internal array. Just like MemoryStream.SetLength() will grow the internal array. Either way the buffer must grow before being used beyond the initialCapacity.
src/NLog.Targets.Syslog/ByteArray.cs
Outdated
public void Append(string buffer, Encoding encoding) | ||
{ | ||
if (string.IsNullOrEmpty(buffer)) | ||
return; | ||
|
||
var byteCount = encoding.GetByteCount(buffer); | ||
memoryStream.SetLength(memoryStream.Length + byteCount); | ||
for (int i = 0; i < buffer.Length; i += encodingBuffer.Length) | ||
{ | ||
int remainingCount = Math.Min(buffer.Length - i, encodingBuffer.Length); | ||
buffer.CopyTo(i, encodingBuffer, 0, remainingCount); | ||
memoryStream.Position += encoding.GetBytes(encodingBuffer, 0, remainingCount, memoryStream.GetBuffer(), (int)memoryStream.Position); | ||
} | ||
} | ||
|
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.
This idea is good, despite reducing readability where this method is called.
The implementation with a fixed buffer size and for doesn't allow, in my opinion, to have great performance advantages, but luckily this can be done operating directly on strings:
public void Append(string s, Encoding encoding)
{
if (string.IsNullOrEmpty(s))
return;
memoryStream.Position += encoding.GetBytes(s, 0, s.Length, memoryStream.GetBuffer(), (int)memoryStream.Position);
When the memoryStream capacity is int.MaxValue and we try writing more than the room left an exception will be thrown: we take this risk without adding protective code since it will be difficult, if not impossible, for a client application, to log a string that is int.MaxValue chars long.
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.
"Luckily" this is a trap. One should never give the string directly to Encoding-object. See the ToCharArray()-call here: https://referencesource.microsoft.com/#mscorlib/system/text/encoding.cs,1075 My version works better.
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.
I saw the ToCharArray before writing your previous comment, but I was trying to find a more readable way to do the same thing: yes you do not allocate, but you copy in a char array to then getbytes...
Probably it would be better to do something like this:
public void Append(string s, Encoding encoding)
{
if (string.IsNullOrEmpty(s))
return;
var length = s.Length;
var byteCount = encoding.GetByteCount(s):
unsafe
{
fixed (char* chars = s)
fixed (byte* buffer = memoryStream.GetBuffer())
{
memoryStream.Position += encoding.GetBytes(chars, length, buffer, byteCount);
}
}
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.
I'm not a big fan of unsafe code, but if you like that then please go ahead. Perform memcpy from one buffer to another is super cheap. Allocating new buffer is expensive (and gives a penalty one have to pay later with an early garbage collection).
3776aea
to
fd0e18a
Compare
Closes #207