Optimize JSON serialization performance

[mus65]

The JSON serializer currently recreates the JsonSerializerOptions every time, which is a huge waste of ressources since the options act as a cache for the serialization metadata.

This changes the serializer to keep the options in a static field instead.

Also some benchmark fixes:

• fix TargetFramework
• add [MemoryAnalyzer]
• fix NullReference
• use BenchmarkSwitcher so use can filter with command line args

Results can be reproduced with:

dotnet run --project benchmark/ --configuration Release -- --filter "*erializeJson*"

Before:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
SerializeJson	7.044 ms	0.0682 ms	0.0638 ms	703.1250	187.5000	11 MB
DeserializeJson	6.973 ms	0.1071 ms	0.1002 ms	703.1250	140.6250	11 MB

After:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
SerializeJson	24.48 us	0.085 us	0.071 us	10.8643	5.3711	178 KB
DeserializeJson	12.86 us	0.034 us	0.030 us	2.9297	0.9003	48 KB

[andreas-hilti]

This is basically a reversal of #389.

[andreas-hilti]

Just understand a bit better: How does this slowness impact you? (After all, it is at least still in the milliseconds in the benchmark case.)

[mtsfoni]

This is basically a reversal of #389.

This changes the serializer to keep the options in a static field instead.

I think we somehow need to manage to keep the options for one full de-/serialization operation without having to make it static. That should solve both problems.

[mus65]

Just understand a bit better: How does this slowness impact you? (After all, it is at least still in the milliseconds in the benchmark case.)

I just noticed that this was very slow while debugging and in production I see small CPU spikes everytime we deserialize an SBOM due to this. Granted it's not a huge issue, but re-creating the JSON options is just wrong, MS even has an analyzer for this.

I think we somehow need to manage to keep the options for one full de-/serialization operation without having to make it static. That should solve both problems.

It would not. We have a web service that consumes SBOMs, so it only deserializes, but across multiple web requests. It shouldn't have to re-create the options each time.

I wonder how exactly #389 did "improve downstream testability"? Are the options changed in some test contexts? Maybe an internal API to reset the options would be a better option?

[andreas-hilti]

@mus65 Compare also #391, where I posed a similar question.
@mtsfoni would need to give the details.
(As a follow-up, I restored the merge performance with #393.)

[andreas-hilti]

Looking at the code, there are two places where Utils.GetJsonSerializerOptions() is unexpected for me:

cyclonedx-dotnet-library/src/CycloneDX.Core/Json/Converters/IdentityListConverter.cs

Line 37 in e8859a7

var serializerOptions = Utils.GetJsonSerializerOptions();

IMO this should just use the provided options.

cyclonedx-dotnet-library/src/CycloneDX.Core/Json/Converters/ToolChoicesConverter.cs

Line 42 in e8859a7

var serializerOptions = Utils.GetJsonSerializerOptions();

If we can't avoid the workaround, we should at least use the copy constructor, i.e.
var serializerOptions = new JsonSerializerOptions(options);
rather than initialising completely new options.

I think we somehow need to manage to keep the options for one full de-/serialization operation without having to make it static.

Apart from these two cases, we are already there as far I can see.

[mus65]

Looking at the code, there are two places where Utils.GetJsonSerializerOptions() is unexpected for me:

cyclonedx-dotnet-library/src/CycloneDX.Core/Json/Converters/IdentityListConverter.cs

Line 37 in e8859a7

var serializerOptions = Utils.GetJsonSerializerOptions();

IMO this should just use the provided options.

yes, as far as I can tell there is no reason to recreate the options here, fixed that with e3872e4.

cyclonedx-dotnet-library/src/CycloneDX.Core/Json/Converters/ToolChoicesConverter.cs

Line 42 in e8859a7

var serializerOptions = Utils.GetJsonSerializerOptions();

If we can't avoid the workaround, we should at least use the copy constructor, i.e.
var serializerOptions = new JsonSerializerOptions(options);
rather than initialising completely new options.

Changed that with e3872e4 . This would still be a performance issue though. Properly fixing this would require deserializing the object manually like this:

while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
{
    if (reader.TokenType == JsonTokenType.PropertyName)
    {
        if(reader.ValueTextEquals("components"))
        {
            toolChoices.Components = JsonSerializer.Deserialize<List<Component>>(ref reader, options);
        }
        else if(reader.ValueTextEquals("services"))
        {
            toolChoices.Services = JsonSerializer.Deserialize<List<Service>>(ref reader, options);
        }
        else
        {
            throw new JsonException();
        }
    }
}

Not great though since that requires changes with new properties being introduced.

I think we somehow need to manage to keep the options for one full de-/serialization operation without having to make it static.

Apart from these two cases, we are already there as far I can see.

I don't quite follow? There are static now (as they imho need to be).

Regarding #391 :

I needed a way to modify the serialization options externally. I added a new feature to the generator (I believe this was also requested in the CLI) to support generating JSON with relaxedEscaping. The problem I ran into is that once the options are created, they become read-only; because the field was static, that broke the test.

Fixed that with 7c7386f by re-recreating the options when UseUnsafeRelaxedJsonEscaping is changed.

edit: had to force push because of missing sign-off.

[andreas-hilti]

I think we somehow need to manage to keep the options for one full de-/serialization operation without having to make it static.

Apart from these two cases, we are already there as far I can see.

I don't quite follow? There are static now (as they imho need to be).

This was just a reply to @mtsfoni on his statement: Namely that already in the current main branch, the options are constructed only once for a full de-/serialization operation.