[dotnet] [bidi] Avoid intermediate JsonDocument allocation to determine unordered discriminator

[nvborisenko]

User description

Motivation and Context

Improve memory allocation.

Before:

| Method               | Mean     | Error    | StdDev   | Gen0    | Gen1    | Allocated |
|--------------------- |---------:|---------:|---------:|--------:|--------:|----------:|
|     LocateNodesAsync | 12.18 ms | 0.060 ms | 0.036 ms | 78.1250 | 31.2500 | 532.29 KB |

After:

| Method               | Mean     | Error    | StdDev   | Gen0    | Gen1    | Allocated |
|--------------------- |---------:|---------:|---------:|--------:|--------:|----------:|
|     LocateNodesAsync | 12.12 ms | 0.062 ms | 0.037 ms | 62.5000 | 15.6250 | 404.28 KB |

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement

---

Description

• Replaced JsonDocument usage with Utf8JsonReader for better performance.

• Introduced GetDiscriminator extension method for cleaner discriminator extraction.

• Reduced memory allocation in JSON deserialization processes.

• Updated multiple converters to use the new GetDiscriminator method.

---

Changes walkthrough 📝

Relevant files

Enhancement

EvaluateResultConverter.cs Optimize `EvaluateResult` deserialization with `Utf8JsonReader` dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/EvaluateResultConverter.cs Replaced JsonDocument with Utf8JsonReader for deserialization. Utilized GetDiscriminator for discriminator extraction. Improved memory efficiency in EvaluateResult deserialization. +4/-5      LogEntryConverter.cs Enhance `LogEntry` deserialization with `Utf8JsonReader`  dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/LogEntryConverter.cs Switched to Utf8JsonReader for LogEntry deserialization. Replaced JsonDocument with GetDiscriminator for type handling. +4/-5      MessageConverter.cs Improve `Message` deserialization using `Utf8JsonReader`  dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/MessageConverter.cs Replaced JsonDocument with Utf8JsonReader for Message deserialization. Used GetDiscriminator for cleaner type extraction. +5/-7      RealmInfoConverter.cs Optimize `RealmInfo` deserialization with `Utf8JsonReader` dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/RealmInfoConverter.cs Updated RealmInfo deserialization to use Utf8JsonReader. Integrated GetDiscriminator for type determination. +10/-11  RemoteValueConverter.cs Enhance `RemoteValue` deserialization with `Utf8JsonReader` dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/RemoteValueConverter.cs Replaced JsonDocument with Utf8JsonReader for RemoteValue deserialization. Enhanced type handling using GetDiscriminator. Improved memory efficiency for string and type-based deserialization. +30/-31  JsonExtensions.cs Introduce `GetDiscriminator` extension for `Utf8JsonReader` dotnet/src/webdriver/BiDi/Communication/Json/Internal/JsonExtensions.cs Added GetDiscriminator extension method for Utf8JsonReader. Simplified discriminator extraction logic for JSON objects. +53/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The GetDiscriminator method doesn't handle the case where the discriminator property exists but has a non-string value. This could lead to unexpected behavior if the JSON structure changes. if (propertyName == name) { discriminator = readerClone.GetString(); break;

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Validate discriminator existence Suggestion Impact: The commit implemented the same validation logic as suggested, but with different implementation. Instead of checking for null before returning, it used the null-coalescing operator with throw to validate the discriminator's existence and throw an exception with a descriptive message when missing. code diff: - return discriminator; + return discriminator ?? throw new JsonException($"Couldn't determine '{name}' descriminator."); The method doesn't handle the case where the discriminator property isn't found in the JSON object. If the property doesn't exist, the method will return null, which could lead to unexpected behavior in the converters that use this method. Consider adding validation to throw a more descriptive exception when the discriminator property is missing. dotnet/src/webdriver/BiDi/Communication/Json/Internal/JsonExtensions.cs [26-52] public static string? GetDiscriminator(this ref Utf8JsonReader reader, string name) { Utf8JsonReader readerClone = reader; if (readerClone.TokenType != JsonTokenType.StartObject) throw new JsonException(); string? discriminator = null; readerClone.Read(); while (readerClone.TokenType == JsonTokenType.PropertyName) { string? propertyName = readerClone.GetString(); readerClone.Read(); if (propertyName == name) { discriminator = readerClone.GetString(); break; } readerClone.Skip(); readerClone.Read(); } + if (discriminator == null) + throw new JsonException($"Required discriminator property '{name}' not found in JSON object."); + return discriminator; } [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a potential bug where missing discriminator properties would return null without error, causing silent failures in the converters. Adding validation with a descriptive exception improves error handling and debugging significantly.	Medium
Learned best practice	Add parameter validation to prevent null reference exceptions when comparing string values The name parameter should be validated to ensure it's not null before using it in the comparison. Add a null check at the beginning of the method to prevent potential NullReferenceException when comparing propertyName == name. dotnet/src/webdriver/BiDi/Communication/Json/Internal/JsonExtensions.cs [26-52] public static string? GetDiscriminator(this ref Utf8JsonReader reader, string name) { + ArgumentNullException.ThrowIfNull(name, nameof(name)); + Utf8JsonReader readerClone = reader; if (readerClone.TokenType != JsonTokenType.StartObject) throw new JsonException(); string? discriminator = null; readerClone.Read(); while (readerClone.TokenType == JsonTokenType.PropertyName) { string? propertyName = readerClone.GetString(); readerClone.Read(); if (propertyName == name) { discriminator = readerClone.GetString(); break; } readerClone.Skip(); readerClone.Read(); } return discriminator; } Apply this suggestion Suggestion importance[1-10]: 6	Low
Update

[RenderMichael]

This does increase the complexity of the JSON handling, I agree that it is worth it. A couple of comments.