JsonSerializer - Serialize Object Properties into StringBuilder #2179

snakefoot · 2017-06-19T23:16:29Z

Prototype of how one could extend Json Serializer to handle object-properties.

Stolen the unit tests from #2128

codecov · 2017-06-19T23:55:54Z

Codecov Report

Merging #2179 into master will decrease coverage by <1%.
The diff coverage is 80%.

@@           Coverage Diff           @@
##           master   #2179    +/-   ##
=======================================
- Coverage      82%     82%   -<1%     
=======================================
  Files         300     304     +4     
  Lines       20623   21154   +531     
  Branches     2483    2537    +54     
=======================================
+ Hits        16853   17282   +429     
- Misses       3150    3236    +86     
- Partials      620     636    +16

…nSerializerV2)

…tring

… MruCache-tests

snakefoot · 2017-06-21T21:35:42Z

@304NotModified Have implemented a simple MruCache and optimized for using StringBuilder by default.

304NotModified · 2017-06-21T21:49:06Z

👍 👍 will try to review it this weekend

snakefoot · 2017-06-22T18:41:08Z

Made a small performance test using a simple DTO with 3 properties (string, integer, enum). And it could serialize 1.000.000 items per second (with very little allocation). Not the fastest in the world, but should cover the needs of NLog.

304NotModified

Nice work! Reviewed it!

304NotModified · 2017-06-22T19:33:14Z

src/NLog/Internal/MruCache.cs

+
+                        if (_dictionary.Count >= _maxCapacity)
+                        {
+                            _dictionary.Clear();


We can do this before the delete prune?

With different check of course

The _dictionary.Clear(); is a fail safe, in case something has gone terrible wrong, and all items has the same version.

304NotModified · 2017-06-22T19:34:30Z

src/NLog/Internal/MruCache.cs

+                }
+                else
+                {
+                    var version = ++_currentVersion;


Postfix ++?

I want "version" to have the updated value. I guess I could write:

var version = _currentVersion += 1 ;

304NotModified · 2017-06-22T19:37:05Z

src/NLog/Internal/MruCache.cs

+                item = default(MruItem);    // Too many people in the same room
+            }
+
+            if (item.Version != _currentVersion)


Please some docs here, thanks! (I guess retrieving the sane is here on purpose)

304NotModified · 2017-06-22T19:38:48Z

src/NLog/Layouts/JsonLayout.cs

+                {
+                    var jsonSerializer = ConfigurationItemFactory.Default.JsonSerializer;
+                    _jsonSerializer = jsonSerializer as NLog.Targets.IJsonSerializerV2;
+                    if (_jsonSerializer == null && jsonSerializer != null)


Maybe nicer if we fix this in the setter?

Well it is lazy-initialization.

fair enough :)

304NotModified · 2017-06-22T19:42:20Z

src/NLog/Targets/DefaultJsonSerializer.cs

        }

-        private static bool IsNumericTypeCode(TypeCode objTypeCode)
+        private static bool IsNumericTypeCode(TypeCode objTypeCode, bool includeDecimals)


Please add some docs why include decimals could be false

304NotModified · 2017-06-22T19:44:57Z

src/NLog/Targets/IJsonSerializer.cs

+        /// </summary>
+        /// <param name="value">The object to serialize to JSON.</param>
+        /// <param name="builder">Output destination.</param>
+        bool SerializeObject(object value, System.Text.StringBuilder builder);


Please document the output value. Is it true if success?

304NotModified · 2017-06-22T19:45:48Z

src/NLog/Targets/IJsonSerializer.cs

@@ -49,4 +49,15 @@ public interface IJsonSerializer
        /// <returns>Serialized value.</returns>
        string SerializeObject(object value);
    }
+
+    internal interface IJsonSerializerV2


I think it should be public and in the NLog namespace, what do you think?

Makes sense to have it public, as it would allow others to optimize for StringBuilder. Not sure about the naming-convention though, by making it internal, then I could just choose a "random" name :).

304NotModified · 2017-06-22T19:47:43Z

Maybe we have to include also some more test cases. 126 missed lines could be better

304NotModified · 2017-06-23T22:33:01Z

src/NLog/Targets/IJsonSerializer.cs

        string SerializeObject(object value);
    }
+
+    internal interface IJsonSerializerV2


Ow a commit has not been added from mobile...

I think it's important to have the interface public, maybe we could move it to the "NLog" namespace? (and mark the other as obsolete)

While the default serialization is good enough for most cases, it should be possible to replace it with e.g. JSON.NET

Yes it will not be super optimal for Json.Net as the implementation would be:

bool SerializeObject(object value, System.Text.StringBuilder sb) { int originalLength = sb.Length; try { StringWriter sw = new StringWriter(sb); JsonSerializer serializer = new JsonSerializer(); using (JsonWriter jsonTextWriter = new JsonTextWriter(sw)) { serializer.Serialize(jsonTextWriter, obj); } return true; } catch { sb.Length = originalLength; return false; } }

snakefoot · 2017-06-25T21:53:12Z

@304NotModified I have now introduced NLog.IJsonConverter and marked NLog.Targets.IJsonSerializer as obsolete.

snakefoot · 2017-06-25T22:22:24Z

IJsonConverter?

Converts object to JSON-string :). Feel free to provide any interface name you want

snakefoot · 2017-06-25T22:28:34Z

Now fixed the obsolete-text to say NLog.IJsonConverter. Good catch.

304NotModified · 2017-06-26T09:56:26Z

I think its better to give the hint of the change property instead of the interface in the obsolete text. Have updated it.

304NotModified · 2017-06-28T09:20:01Z

merged! Thanks!

snakefoot force-pushed the JsonSerializeObjectProperties branch 3 times, most recently from 401727a to c07d817 Compare June 19, 2017 23:42

JsonSerializer - Serialize Object Properties into StringBuilder (IJso…

86baecb

…nSerializerV2)

snakefoot force-pushed the JsonSerializeObjectProperties branch 4 times, most recently from 9134aba to 89cc2d2 Compare June 20, 2017 21:27

JsonSerializer - Improved performance for EnumAsString and IntegerAsS…

37b4528

…tring

snakefoot force-pushed the JsonSerializeObjectProperties branch from 89cc2d2 to 37b4528 Compare June 21, 2017 05:09

JsonSerializer - Refactor SerializeObject into smaller methods. Added…

775d81f

… MruCache-tests

snakefoot force-pushed the JsonSerializeObjectProperties branch from b7f8f2f to 638a699 Compare June 21, 2017 21:32

JsonSerializer - Make use of LateBoundMethod to optimize performance

3b77913

snakefoot force-pushed the JsonSerializeObjectProperties branch from 638a699 to 3b77913 Compare June 21, 2017 21:46

snakefoot force-pushed the JsonSerializeObjectProperties branch 7 times, most recently from decb48e to a3f3d44 Compare June 22, 2017 18:37

snakefoot force-pushed the JsonSerializeObjectProperties branch from a3f3d44 to 8c2ef71 Compare June 22, 2017 18:55

304NotModified reviewed Jun 22, 2017

View reviewed changes

snakefoot force-pushed the JsonSerializeObjectProperties branch 2 times, most recently from 13c3e86 to 1f98a75 Compare June 22, 2017 20:59

snakefoot force-pushed the JsonSerializeObjectProperties branch from f6b60d0 to 9c50e98 Compare June 22, 2017 22:23

JsonSerializer - Skip HashSet allocation when handling simple DTOs

2d5baf8

snakefoot force-pushed the JsonSerializeObjectProperties branch from 9c50e98 to 2d5baf8 Compare June 22, 2017 22:35

304NotModified approved these changes Jun 23, 2017

View reviewed changes

304NotModified added the feature label Jun 23, 2017

304NotModified reviewed Jun 23, 2017

View reviewed changes

304NotModified mentioned this pull request Jun 23, 2017

JSON serializer: support objects, improvements and formatting, add memoryCache #2128

Closed

10 tasks

snakefoot force-pushed the JsonSerializeObjectProperties branch from 85df7af to 20f5c75 Compare June 25, 2017 21:52

snakefoot force-pushed the JsonSerializeObjectProperties branch 4 times, most recently from 08b12aa to 865abcd Compare June 25, 2017 22:15

JsonSerializer - Introduced NLog.IJsonConverter as public interface

062ec3f

snakefoot force-pushed the JsonSerializeObjectProperties branch from 865abcd to 062ec3f Compare June 25, 2017 22:27

moved code for potential conflicts, updated Obsolete text

278d41d

304NotModified added this to the 4.5 milestone Jun 28, 2017

304NotModified merged commit 7399c93 into NLog:master Jun 28, 2017

304NotModified modified the milestones: 4.5, 4.5 beta ? Oct 10, 2017

snakefoot deleted the JsonSerializeObjectProperties branch October 10, 2017 20:42

snakefoot mentioned this pull request Nov 23, 2017

NLog 4.5 changelog #2325

Closed

snakefoot mentioned this pull request Aug 18, 2021

Fix CultureNotFoundException with Globalization Invariant Mode #4519

Merged

snakefoot modified the milestones: 4.5 beta ?, 4.5 Aug 22, 2023

snakefoot added the json / json-layout label Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JsonSerializer - Serialize Object Properties into StringBuilder #2179

JsonSerializer - Serialize Object Properties into StringBuilder #2179

snakefoot commented Jun 19, 2017 •

edited

Loading

codecov bot commented Jun 19, 2017 •

edited

Loading

snakefoot commented Jun 21, 2017

304NotModified commented Jun 21, 2017

snakefoot commented Jun 22, 2017 •

edited

Loading

304NotModified left a comment

304NotModified Jun 22, 2017

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017 •

edited

Loading

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017

304NotModified Jun 23, 2017

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017

304NotModified Jun 22, 2017

snakefoot Jun 22, 2017

304NotModified commented Jun 22, 2017

304NotModified Jun 23, 2017

snakefoot Jun 25, 2017 •

edited

Loading

snakefoot commented Jun 25, 2017 •

edited

Loading

snakefoot commented Jun 25, 2017 •

edited

Loading

snakefoot commented Jun 25, 2017

304NotModified commented Jun 26, 2017

304NotModified commented Jun 28, 2017

JsonSerializer - Serialize Object Properties into StringBuilder #2179

JsonSerializer - Serialize Object Properties into StringBuilder #2179

Conversation

snakefoot commented Jun 19, 2017 • edited Loading

codecov bot commented Jun 19, 2017 • edited Loading

Codecov Report

snakefoot commented Jun 21, 2017

304NotModified commented Jun 21, 2017

snakefoot commented Jun 22, 2017 • edited Loading

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

snakefoot Jun 22, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

304NotModified commented Jun 22, 2017

Choose a reason for hiding this comment

snakefoot Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

snakefoot commented Jun 25, 2017 • edited Loading

snakefoot commented Jun 25, 2017 • edited Loading

snakefoot commented Jun 25, 2017

304NotModified commented Jun 26, 2017

304NotModified commented Jun 28, 2017

snakefoot commented Jun 19, 2017 •

edited

Loading

codecov bot commented Jun 19, 2017 •

edited

Loading

snakefoot commented Jun 22, 2017 •

edited

Loading

snakefoot Jun 22, 2017 •

edited

Loading

snakefoot Jun 25, 2017 •

edited

Loading

snakefoot commented Jun 25, 2017 •

edited

Loading

snakefoot commented Jun 25, 2017 •

edited

Loading