Skip to content
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

reuse the Newtonsoft.Json serializer #5197

Conversation

Aaronontheweb
Copy link
Member

Rather than call JsonConvert on each instance, which creates a new JsonSerializer instance internally - let's just re-use the one created when the NewtonsoftJsonSerializer is instantiated.

Noticed this while working on DU serialization.

Before

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1110 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
  [Host]     : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
  DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT

Method Mean Error StdDev Gen 0 Allocated
Json_serializer_message_roundtrip 27.83 μs 0.167 μs 0.140 μs 3.2000 13 KB

After

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1110 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
  [Host]     : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
  DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT

Method Mean Error StdDev Gen 0 Allocated
Json_serializer_message_roundtrip 26.36 μs 0.125 μs 0.104 μs 3.0000 13 KB

Looks like it helped a small amount (looking at the Gen 0 numbers)

@Aaronontheweb
Copy link
Member Author

Added this when I was looking at DU handling for #5196

@Aaronontheweb
Copy link
Member Author

Akka.DistributedData.Tests.Serialization.ReplicatedDataSerializerSpec.ReplicatedDataSerializer_should_serialize_GSet

Failed on all platforms - which makes me suspicious

@to11mtm
Copy link
Member

to11mtm commented Aug 12, 2021

There are thread hazards around re-using a JsonSerializer with certain configurations, IIRC specifically around TypeName resolution and/or Preserving object references. When I did #4929 I found any attempts to re-use the serializer would result in a lot of red on my unit tests. There's a PR to fix in Json.NET but it has been in limbo for some time..

@Aaronontheweb
Copy link
Member Author

There are thread hazards around re-using a JsonSerializer with certain configurations, IIRC specifically around TypeName resolution and/or Preserving object references. When I did #4929 I found any attempts to re-use the serializer would result in a lot of red on my unit tests. There's a PR to fix in Json.NET but it has been in limbo for some time..

That's great to know @to11mtm - I'll de-milestone this PR and mark it as "on hold" then. To be revisited later.

@Aaronontheweb Aaronontheweb removed this from the 1.4.24 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants