Use Json.NET support for Streams in NewtonSoftJsonSerializer #5376
Conversation
There was a problem hiding this comment.
Looks great - seems like a safe change; will wait on CI to tell me otherwise if I'm wrong.
FYI: we just merged in .NET 6 support today and it appears to have caused some regressions in the way the .NET Threadpool behaves. I'm still investigating that, but if there are unrelated unit tests that fail on your PR don't panic - we have a number of chronically racy tests that we've been gradually fixing but it looks like .NET 6 made that problem worse.
| @@ -47,6 +56,16 @@ public void Json_serializer_message_roundtrip() | |||
| } | |||
| } | |||
|
|
|||
| [Benchmark(OperationsPerInvoke = Operations)] | |||
| public void Json_serializer_large_message_roundtrip() | |||
| using (var writer = new StreamWriter(stream)) | ||
| { | ||
| _serializer.Serialize(writer, obj); | ||
| return stream.ToArray(); |
| using (var reader = new JsonTextReader(r)) | ||
| { | ||
| object res = _serializer.Deserialize(reader); | ||
| return TranslateSurrogate(res, this, type); |
|
And thanks for adding the benchmark too - that made this PR much easier to review. |
|
Ah, looks like all of the F# Serialization specs just blew up |
There was a problem hiding this comment.
Looks like we broke the F# DU serialization tests.
cc @to11mtm - I can't recall of this is the case or not, but is it safe to re-use the same _serializer instance on multiple threads concurrently?
| using (var r = new StreamReader(stream)) | ||
| using (var reader = new JsonTextReader(r)) | ||
| { | ||
| object res = _serializer.Deserialize(reader); |
There was a problem hiding this comment.
So the Settings are passed in properly here when the _serializer is created - is the issue that the _serializer class isn't meant to be used concurrently or is it that JsonConvert is doing something differently than _serializer.Deserialize?
There was a problem hiding this comment.
Ah, looks like the StreamWriter used in ToBinary above needs to be flushed/disposed before attempting to return the byte[]. Will push an updated commit after re-running tests locally.
There was a problem hiding this comment.
Can you re-run the benchmarks also?
| { | ||
| for (int i = 0; i < Operations; i++) | ||
| { | ||
| var bytes = json.ToBinary(message); |
There was a problem hiding this comment.
I totally missed this copypasta. Working on fix and will include fresh runs of benchmarks.
| using (var r = new StreamReader(stream)) | ||
| using (var reader = new JsonTextReader(r)) | ||
| { | ||
| object res = _serializer.Deserialize(reader); |
There was a problem hiding this comment.
We cannot trust the Thread safety of JsonSerializer in it's present state. You either have to create a new Serializer each time or use the static methods (which do so themselves)
There was a problem hiding this comment.
Dang, that's disappointing. Why does the current implementation hold on to a reference of an instance of the serializer then?
There was a problem hiding this comment.
Dang, that's disappointing. Why does the current implementation hold on to a reference of an instance of the serializer then?
No idea; we probably don't need to have it anymore. Although having a ThreadStatic reference to it might not be the worst thing in the world, in combination with your changes.
|
I'm going to have to put this work on hold. There's a surprising performance difference between StringBuilder+StringWriter and MemoryStream+StreamWriter and I don't have time in the near term to work it out. I'll come back to this in a bit. |
…memory during serialization/deserialization Also included separate benchmarks to identify performance differences between serialization and deserialization
… JsonSerializer and improve performance for serializing and deserializing large objects. Based on [recommendation from Json.NET docs](https://www.newtonsoft.com/json/help/html/Performance.htm#MemoryUsage)
Head branch was pushed to by a user without write access
465b6b8 to
06368e6
Compare
|
This latest push is a bit deeper than the initial attempt at updating the Much to my surprise, using I broke out the different duties that were being handled in Latest benchmarksThe initial work included a bug that did not properly flush the stream in In summary, I worked to get performance parity with the "before" for small messages and an improvement for large messages. BeforeBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-FMTJAV : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
AfterBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-KKWJDG : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
|
|
Looks like the |
|
Looks like it's a very related issue for the |
|
Reviewed more specifics of how Json.NET's internal DefaultReferenceResolver works and implemented a suitable replacement here. The IReferenceResolver implementation in Json.NET is one of the main issues with BenchmarksRe-ran benchmarks against this update and the results look about as good as they did before BeforeBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-CDLPJT : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
AfterBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-HWSFHK : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
|
|
@to11mtm what do you make of these latest changes? |
(This was formerly handled by TranslateSurrogate())
|
One more update to resolve the remaining failing tests. Had to reimplement the function that BenchmarksThe difference in results on BeforeBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-VTZTAU : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
AfterBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-MKUYFU : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
|
|
Ah, caught my problem. My last commit moved the BenchmarksBeforeBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-WBVURS : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
AfterBenchmarkDotNet=v0.13.1, OS=macOS Big Sur 11.6.1 (20G224) [Darwin 20.6.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
Job-JVIIBH : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
IterationCount=10
|
| string data = JsonConvert.SerializeObject(obj, Formatting.None, Settings); | ||
| byte[] bytes = Encoding.UTF8.GetBytes(data); | ||
| return bytes; | ||
| var stringBuilder = new StringBuilder(256); |
There was a problem hiding this comment.
Would it be possible to pull in my changes for Pooling? If we can get this all to work, it would be nice to get both wins in place (especially since I've been waiting for merge on it for some time.)
|
|
||
| public partial class NewtonSoftJsonSerializer | ||
| { | ||
| internal class PrimitiveNumberConverter : JsonConverter, IObjectConverter |
| switch (primitiveType) | ||
| { | ||
| case 'I': | ||
| convertedValue = int.Parse(numberString, NumberFormatInfo.InvariantInfo); |
There was a problem hiding this comment.
@Aaronontheweb Is it worth getting a TODO comment here reminding us that when we can multitarget, to use Span overloads?
| { | ||
| private readonly JsonSerializer _serializer; | ||
| private readonly ThreadLocal<JsonSerializer> _serializer; |
There was a problem hiding this comment.
I'm not a fan of ThreadLocal for a number of reasons, but I know that we have other challenges at present. @Aaronontheweb is there a polite way to get this fixed upstream?
There was a problem hiding this comment.
Getting this fixed upstream as in:
- Newtonsoft.Json itself?
- Akka.NET's overall serialization system?
|
|
||
| public partial class NewtonSoftJsonSerializer | ||
| { | ||
| internal class ReferenceResolver : IReferenceResolver |
There was a problem hiding this comment.
I'm wondering whether adapting This IRefrenceResolver fix, or this one would be better overall and help us avoid the threadlocal? My worry is the performance as well as cognitive overhead of WeakReference in this implementation.
Includes a new benchmark test for serializing and deserialzing especially large messages..
Before
After