Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocates less memory #1695

snakefoot · 2016-10-09T07:33:11Z

These three renderers are using the invariant culture. - StringBuilder Append without allocation

This change is

codecov-io · 2016-10-09T08:35:36Z

Current coverage is 80% (diff: 100%)

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

@@             master      #1695   diff @@
==========================================
  Files           274        274          
  Lines         16871      16905    +34   
  Methods        2666       2669     +3   
  Messages          0          0          
  Branches       1871       1875     +4   
==========================================
+ Hits          13511      13543    +32   
- Misses         2928       2929     +1   
- Partials        432        433     +1

Powered by Codecov. Last update 33b0bea...182c9dd

304NotModified

What kind of garbage are we talking about? Garbage collector garbage?

304NotModified · 2016-10-09T17:47:11Z

src/NLog/Internal/StringBuilderExt.cs

+
+            // Calculate length of integer when written out
+            int length;
+            if (value >= 10000)


uhm, what about value / 10 ?

Less-than should be faster than division, and requires fewer iterations when larger values.

But I can revert, when testing then the profiler never picked up the ThreadIdLayoutRenderer whether using division-loop or if-less-than

That sounds like a premature optimization? Also we have now more less-than operations needed. My proposal is to replace the whole block with a single division (and some rounding up or down, dunno)

Have removed the optimization and squashed the commits

304NotModified · 2016-10-09T17:48:03Z

tests/NLog.UnitTests/Internal/StringBuilderExtTests.cs

+        [InlineData(1234567890)]
+        [InlineData(int.MaxValue)]
+        [InlineData(int.MinValue)]
+        void TestAppendInvariant(int input)


we're missing negative numbers?

They are inside the test-method. I subtract the value from 0.

missed that. Maybe rename the method?

Have no special opinion. Have a good method name?

TestAppendInvariant_positive_and_negative :P

snakefoot · 2016-10-09T17:52:39Z

Changed the title from without garbage to without allocation

304NotModified · 2016-10-09T18:12:00Z

PS: I'm renaming the PRs so it is more suitable for the changelog.

snakefoot force-pushed the ThreaIdLayoutAppendInvariant branch from 87007b6 to 44e867d Compare October 9, 2016 08:00

304NotModified reviewed Oct 9, 2016

View reviewed changes

snakefoot changed the title ~~ThreadIdLayoutRenderer - StringBuilder Append without garbage~~ ThreadIdLayoutRenderer - StringBuilder Append without allocation Oct 9, 2016

ThreadIdLayoutRenderer - StringBuilder Append without garbage

182c9dd

snakefoot force-pushed the ThreaIdLayoutAppendInvariant branch from c5d8791 to 182c9dd Compare October 9, 2016 18:08

304NotModified added the performance label Oct 9, 2016

304NotModified changed the title ~~ThreadIdLayoutRenderer - StringBuilder Append without allocation~~ Performance: ThreadIdLayoutRenderer allocations less memory Oct 9, 2016

304NotModified changed the title ~~Performance: ThreadIdLayoutRenderer allocations less memory~~ Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocations less memory Oct 9, 2016

304NotModified added this to the 4.3.10 milestone Oct 9, 2016

304NotModified merged commit 91e0391 into NLog:master Oct 9, 2016

snakefoot deleted the ThreaIdLayoutAppendInvariant branch October 9, 2016 23:16

snakefoot changed the title ~~Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocations less memory~~ Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocates less memory Oct 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocates less memory #1695

Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocates less memory #1695

snakefoot commented Oct 9, 2016 •

edited by 304NotModified

codecov-io commented Oct 9, 2016 •

edited

304NotModified left a comment

304NotModified Oct 9, 2016

snakefoot Oct 9, 2016

snakefoot Oct 9, 2016

304NotModified Oct 9, 2016

snakefoot Oct 9, 2016

304NotModified Oct 9, 2016

snakefoot Oct 9, 2016

304NotModified Oct 9, 2016

snakefoot Oct 9, 2016

304NotModified Oct 9, 2016 •

edited

snakefoot commented Oct 9, 2016

304NotModified commented Oct 9, 2016

Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocates less memory #1695

Performance: Counter/ProcessId/ThreadId-LayoutRenderer allocates less memory #1695

Conversation

snakefoot commented Oct 9, 2016 • edited by 304NotModified

codecov-io commented Oct 9, 2016 • edited

Current coverage is 80% (diff: 100%)

304NotModified left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

304NotModified Oct 9, 2016 • edited

Choose a reason for hiding this comment

snakefoot commented Oct 9, 2016

304NotModified commented Oct 9, 2016

snakefoot commented Oct 9, 2016 •

edited by 304NotModified

codecov-io commented Oct 9, 2016 •

edited

304NotModified Oct 9, 2016 •

edited