-
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
NLog - ValueSerializer - Faster integer and enum #2326
NLog - ValueSerializer - Faster integer and enum #2326
Conversation
@@ -59,6 +59,8 @@ private ValueSerializer() | |||
private const int MaxValueLength = 512 * 1024; | |||
private const string LiteralFormatSymbol = "l"; | |||
|
|||
private readonly MruCache<Enum, string> _enumCache = new MruCache<Enum, string>(10000); |
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.
isn't that a bit big? (10000)
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.
No idea. Just chose the same as for the DefaultJsonSerializer. Maybe there are more than 10000 enums in the world?
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.
Should we change it to 5000 for the both of them ?
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.
was more thinking of 1000. Or even lower. Could not image your using 1000 different enum values in 1 project.
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.
Now changed it to 1500 (also for the Json-Converter)
@304NotModified Have cheated a little, since Integer.ToString is pretty much the same in all languages (when no format-string). And have also performed the same cheating for Enum (when no format string). |
ce9df47
to
8a42888
Compare
No problem if it's document in the source ;) Otherwise it could get lost in refactoring |
8a42888
to
fa90361
Compare
Added a comment about the special optimization. |
6878b1c
to
394cb85
Compare
do we need to pref test this? Or are you sure it's faster? |
Codecov Report
@@ Coverage Diff @@
## master #2326 +/- ##
=======================================
- Coverage 82% 82% -<1%
=======================================
Files 317 317
Lines 22676 22716 +40
Branches 2780 2784 +4
=======================================
+ Hits 18588 18611 +23
- Misses 3386 3404 +18
+ Partials 702 701 -1 |
394cb85
to
d3d1862
Compare
The enum printing is 100 pct faster, and the integer printing is 50 pct. faster (and both without allocations). |
@304NotModified Should be ready for review (again) :) |
great! Merged! |
Trying to resolve #2323