-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 allocations optimization (up to 15% speed improvement) #1871
Filetarget allocations optimization (up to 15% speed improvement) #1871
Conversation
@snakefoot as your are active on the FileTarget, your opinion please :) @nazim9214 thanks! |
int newLineBytesCount = this.Encoding.GetByteCount(this.NewLineChars); | ||
byte[] bytes = new byte[textBytesCount + newLineBytesCount]; | ||
this.Encoding.GetBytes(text, 0, text.Length, bytes, 0); | ||
this.Encoding.GetBytes(this.NewLineChars, 0, this.NewLineChars.Length, bytes, textBytesCount); |
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.
As the NewLineChars & Encoding almost never changes while running, I was thinking to "cache" this one (static
variable). I noticed before while performance testing that setting a fixed bytes for the newline improves the performance ~10-20%. (but with a limited test)
Your thoughts please :)
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 wrote a version where 'line ending bytes' are computed only on setting Encoding/LineEnding property of FileTarget (as i understand user can change this properties at runtime in some way?).
My tests on small and medium message sizes show same throughput as in this implementation.
So i think we shouldn't add more complexity to codebase, because there isn't any significant benefits.
Current coverage is 81% (diff: 100%)@@ master #1871 diff @@
==========================================
Files 276 276
Lines 18519 18652 +133
Methods 2860 2862 +2
Messages 0 0
Branches 2143 2142 -1
==========================================
+ Hits 15097 15180 +83
- Misses 2926 2969 +43
- Partials 496 503 +7
|
It is a good optimization of the old code path without breaking interfaces. Will of course conflict with my PR #1799 that introduces a new code-path that allows reuse of MemoryStream + byte[]-array + StringBuilder. But I can fix that. |
Instead of making an array of byte-array to make an exact MemoryAllocation could it not just be:
Of course this very crude calculation will be cheated when having very uneven string-lengths. And the first string is the shortest one. |
Yes, we could use this approach.
I'm not sure which of the approaches select, your thoughts please @304NotModified @snakefoot |
I think random is more important, but also not sure if <5% improvement is worth the change |
It will just be easier for me to merge with #1799, if not needing to keep track of an array of byte-arrays. I guess in NLog 5.0 then the old code-path will be removed completely, and the MemoryStream will just be reused instead of being allocated and thrown away constantly. |
Ok, then i'll rewrite this piece of code, as you proposed. |
👍 thanks in advance! |
Thanks again! |
Removed new string allocation in GetBytesToWrite method. Now we are get count of bytes for text and newline characters and write directly into byte array.
Allocate MemoryStream directly with required buffer size.
Performance results for this test https://github.com/NLog/NLog-Performance-tests/blob/master/NLogPerformance
with following parameters:
This change is