Use .net 6 json source generators for performance

[gfs]

https://devblogs.microsoft.com/dotnet/try-the-new-system-text-json-source-generator/

Benchmarks show 30-40% faster serialization performance.

[WingZer0o]

Would this require a version bump of .NET 5 to .NET 6? Are their any specific areas that need this done?

[gfs]

The main branch is actually behind. On Release 2.3 net 6 is the lowest version so this should be doable now. I think it mostly requires jut adding annotations to the correct objects - there's two different ways that the objects get serialized - either for JSON format output (though sarif is recommended and I don't think this would help in that respect as the sarif SDK uses newtonsoft rather than System.Text.JSON) and for the internal serialization used to store collected data in the sqlite database.

For regular json serialization it looks to me like we are using Newtonsoft so the method would have to be converted to System.Text.Json here:

AttackSurfaceAnalyzer/Cli/AttackSurfaceAnalyzerClient.cs

Line 521 in ff468b9

private static ASA_ERROR ExportCompareResults(ConcurrentDictionary<(RESULT_TYPE, CHANGE_TYPE), List<CompareResult>> resultsIn, ExportOptions opts, string baseFileName, string analysesHash, IEnumerable<AsaRule> rules)

as well as annotating the appropriate objects.

For the second aspect I think the things that need to be annotated are the things that inherit from Collect/Compare objects. I think this also requires updating the JsonUtils helper to use System.Text.Json instead of Newtonsoft to actually take advantage of the perf uplift.

AttackSurfaceAnalyzer/Lib/Objects/CollectObject.cs

Line 12 in ff468b9

public abstract class CollectObject

AttackSurfaceAnalyzer/Lib/Utils/JsonUtils.cs

Line 38 in ff468b9

public static CollectObject? Hydrate(string serialized, RESULT_TYPE type)

[WingZer0o]

Something that I discovered that is necessary to complete this in a semi similar fashion with the Dehydration of CollectObjects is https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/polymorphism?pivots=dotnet-8-0 this feature set is not supported in .NET 6 and I had to remove the .NET 6 support to test out the feature.

After removing the .NET 6 support I am receiving this error even on the derived class of CertificateObject.

System.NotSupportedException: Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. Type 'Microsoft.CST.AttackSurfaceAnalyzer.Objects.SerializableCertificate'. Path: $.Certificate | LineNumber: 0 | BytePositionInLine: 31. ---> System.NotSupportedException: Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. Type 'Microsoft.CST.AttackSurfaceAnalyzer.Objects.SerializableCertificate'.

Is this still something you want to proceed with?

[gfs]

For the first issue, it should be fine to drop net 6, it ends support later this year anyway. Switching to NET 8 only I think would be fine.

For the second issue, I think the issue may be both Newtonsoft and System.Text.Json use [JsonConstructor] as the name of the attribute to specify a specific parameterized constructor for serialization (From this table it looks like they used the same name for the tag: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-9-0#specify-constructor-to-use-when-deserializing) - I hypothesize it may be the case that SerializedCertificate is importing newtonsoft rather than system.text.json, so when it gets to try to serialize the SerializableCertificateObject it won't recognize the attribute and thus can't determine what constructor to us?

[WingZer0o]

I can have a look into this pretty soon maybe tonight or this weekend. Let me double check the namespace.

[WingZer0o]

So I annotated and replaced all of the serialization and deserialization in JsonUtil and got the hydration test suite to pass with green lights.

I will slowly pick away at the rest of the application.

Do we want to completely uninstall Newtonsoft as a dependency since this switch over to System.Text.Json is happening?

Also what about some of these JsonSeralization/Deseralization methods that are taking options? I am importing the System.Text.Json one and it seems like lots of these options don't exist.

[WingZer0o]

Sorry about the multiple replies. I realized this morning I misread your question. To continue supporting .NET 6 there is a way to serialize derived classes that should work across 6, 7, and 8 that might be a good idea to proceed with. It’s the .NET 6 version. I would have to refactor some code I did last night to fix that. Either way please do let me know what the suggestion is to move forward.

…

On Thu, May 30, 2024 at 3:36 PM Gabe Stocco ***@***.***> wrote: For the first issue, I might need to get back to you on removing net 6 support - since NET 8 is the new LTS that may be possible to switch to Net 8 only (which also may allow some new language features). For the second issue, I think the issue may be both Newtonsoft and System.Text.Json use [JsonConstructor] as the name of the attribute to specify a specific parameterized constructor for serialization (From this table it looks like they used the same name for the tag: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-9-0#specify-constructor-to-use-when-deserializing) - I hypothesize it may be the case that SerializedCertificate is importing newtonsoft rather than system.text.json, so when it gets to try to serialize the SerializableCertificateObject it won't recognize the attribute and thus can't determine what constructor to us? — Reply to this email directly, view it on GitHub <#625 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BEG2SNEZ6PYZ6T6NI6G2O63ZE55U3AVCNFSM5KQ4PU5KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJUGA3TINBSGE2A> . You are receiving this because you commented.Message ID: ***@***.***>

[gfs]

Do we want to completely uninstall Newtonsoft as a dependency since this switch over to System.Text.Json is happening?

I think the goal ideally is to migrate all the direct ASA code to use System.Text.Json, though, it can't be eliminated as a transitive dependency because its required by the Sarif SDK.

Also what about some of these JsonSeralization/Deseralization methods that are taking options? I am importing the System.Text.Json one and it seems like lots of these options don't exist.

Do you mean like the Should Serialize methods like this one?

AttackSurfaceAnalyzer/Lib/Objects/CompareResult.cs

Line 96 in ff468b9

public bool ShouldSerializeDiffs()

There is an alternate way to have custom logic but only on NET 7+. https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/custom-contracts

I think we do want to keep the behavior that empty fields aren't serialized if possible, it saves a fair few bytes, it looks like for .NET 6 it is possible to not serialize default values, which is likely the first thing to try. If there's other places in particular you're concerned about not having an equivalent option and you can point them out I may be above to advise more explicitly how I'd prefer to handle them.

To continue supporting .NET 6 there is a way to serialize derived
classes that should work across 6, 7, and 8 that might be a good idea to
proceed with.

That sounds great, if it can work across all the currently supported frameworks that's I think simpler to merge, but its not I think a hard requirement as there's only a couple months of support left for .NET 6/7 anyway.

[WingZer0o]

I was mostly referring to the Serializer settings that are accpected as a parameter to the JsonSerializer for Newtonsoft like the following in WriteMonitorJson.

JsonSerializer serializer = JsonSerializer.Create(new JsonSerializerSettings()
{
    Formatting = Formatting.Indented,
    NullValueHandling = NullValueHandling.Ignore,
    DefaultValueHandling = DefaultValueHandling.Ignore,
    Converters = new List<JsonConverter>() { new StringEnumConverter() }
});

[gfs]

I think it would be good to preserve similar functionality with system text.json ahen possible. I think there are equivalent settings for each of those in the link I sent above for converting from newtonsoft to system.text..

[WingZer0o]

Perhaps I'm a little out of depth with that task at hand to convert these over, but I am not seeing the same name properties within JsonSerializerOptions as JsonSerializerSettings.

[gfs]

@WingZer0o That's okay then, I can handle putting those back when you put up a PR to review if you want to skip it then. Finding the equivalent for each is a bit of a scavenger hunt and they're not always 1:1, but this page has the mappings for most Newtonsoft settings: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-9-0

For example the closest equivalent to IgnoreDefaultValues is by setting the DefaultIgnoreCondition value in the settings: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/ignore-properties#ignore-all-default-value-properties.