Add StringBuilder pooling in NewtonsoftJsonSerializer

[to11mtm]

This PR Gives us a boost on NewtonsoftJsonSerializer's Serialization Throughput.

By lowering memory pressure, we see a ~5% gain under a single thread, and >25% gain under a little bit of Task pressure:

Edit- after Fixes, Some tricky thread things going on in JsonSerializer...:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Pooling	42.30 ms	0.343 ms	0.304 ms	11000.0000	-	-	49.67 MB
NoPooling	44.76 ms	0.310 ms	0.259 ms	15833.3333	83.3333	-	71.26 MB
Pooling_MultiTasks	126.83 ms	1.899 ms	1.683 ms	115500.0000	3750.0000	-	496.71 MB
NoPooling_MultiTasks	176.41 ms	3.517 ms	3.763 ms	173000.0000	15000.0000	-	712.63 MB

[to11mtm]

I tried to make sure to document the settings here. I would love some feedback on the defaults, I'm not sure if maxsize is too low or not. I think MinSize is fair here; I tried 1024B and the benchmarks came out about the same.

[to11mtm]

so, the idea here is to make sure that even if the user wants formatting for -their- serializer, we have our own with no formatting, so that we avoid making a new serializer every time.

[to11mtm]

I made this static because it makes it harder for people to accidentally miswire things (i.e. by accidentally applying to Settings on the instance when each created JsonSerializerSettings needs to have it's inits run.)

[Aaronontheweb]

Looks very promising @to11mtm

[to11mtm]

Well, this tripped a bunch of specs, but different depending on environment... I'm guessing the pooling initialization is in some cases causing some of these specs to time out.

[Aaronontheweb]

@to11mtm no, these look like some of the usual racy specs to me

[Aaronontheweb]

Ready to give this a real review when you say so @to11mtm

[to11mtm]

Ready to give this a real review when you say so @to11mtm

I think this is GTG, although I wonder if we can/should change the max-pool-size.

The challenge is that payloads over 32k will wind up invalidating the cache, but larger sizes can wind up on the LOH because of the way StringBuilder.Clear() works.

This really isn't a bad bad thing TBH, but worth deciding whether we want to do that.

[thzinc]

I came across this while researching serialization issues of my own and arriving at NewtonSoftJsonSerializer. I noticed that the ToBinary and FromBinary implementations here could probably benefit from avoiding string altogether by using streams. (Ref from performance tips docs)

I'm testing out the following changes:

        public override byte[] ToBinary(object obj)
        {
            using (var stream = new MemoryStream())
            using (var writer = new StreamWriter(stream))
            {
                _serializer.Serialize(writer, obj);
                return stream.ToArray();
            }
        }
        // snip
        public override object FromBinary(byte[] bytes, Type type)
        {
            using (var stream = new MemoryStream(bytes))
            using (var r = new StreamReader(stream))
            using (var reader = new JsonTextReader(r))
            {
                object res = _serializer.Deserialize(reader);
                return TranslateSurrogate(res, this, type);
            }
        }

I was planning to submit a PR with this change, but I see that obviously it would clash with this work. Is there some other benefit to using this StringBuilder implementation over Json.NET's stream operations?

[Aaronontheweb]

I came across this while researching serialization issues of my own and arriving at NewtonSoftJsonSerializer. I noticed that the ToBinary and FromBinary implementations here could probably benefit from avoiding string altogether by using streams. (Ref from performance tips docs)

I'm testing out the following changes:

        public override byte[] ToBinary(object obj)
        {
            using (var stream = new MemoryStream())
            using (var writer = new StreamWriter(stream))
            {
                _serializer.Serialize(writer, obj);
                return stream.ToArray();
            }
        }
        // snip
        public override object FromBinary(byte[] bytes, Type type)
        {
            using (var stream = new MemoryStream(bytes))
            using (var r = new StreamReader(stream))
            using (var reader = new JsonTextReader(r))
            {
                object res = _serializer.Deserialize(reader);
                return TranslateSurrogate(res, this, type);
            }
        }

I was planning to submit a PR with this change, but I see that obviously it would clash with this work. Is there some other benefit to using this StringBuilder implementation over Json.NET's stream operations?

IMHO, send this PR in anyway so we can benchmark it. Would love to see what kind of difference this makes. I don't think neither @to11mtm nor myself have a strong opinion about what approach we go with - we just care that the approach is compatible with the wire / API and is faster than what we have today.

[thzinc]

@Aaronontheweb submitted #5376

[Aaronontheweb]

... I honestly have no idea why we haven't merged this already though. Must have been me getting distracted with other stuff.

[Aaronontheweb]

Excellent, thank you @thzinc - I'm about to publish Akka.NET v1.4.28 but I will take a look at your PR!

[to11mtm]

... I honestly have no idea why we haven't merged this already though. Must have been me getting distracted with other stuff.

Or you want me to go away. That said, Merge this and I'll do the Threadpool deed. :)

[Aaronontheweb]

LGTM

[Aaronontheweb]

At some point this will get folded into the core runtime when we move on from .NET Standard 2.0, IIRC

[Aaronontheweb]

LGTM