New JsonValue derived class for JSON primitives #116798
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new JsonValueOfJsonPrimitive class to optimize handling of JSON primitive values by storing raw UTF-8 bytes and escaping info, refactors reader and converter logic to use it, and adds comprehensive tests for primitive behaviors and conversions.
- Add
JsonValueOfJsonPrimitiveto store only needed data for primitives and preserve original escaping inWriteTo. - Update
JsonValueConverter,JsonDocument, andUtf8JsonReaderto delegate GUID parsing to a newJsonReaderHelper.TryGetValuehelper. - Extend tests in
JsonValueTests.csto cover deserialization,TryGetValue,WriteTo, deep-cloning, and conversion for all primitive types using a newWrappedT<T>wrapper.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonValueTests.cs | Added tests for primitive JSON values (string, bool, number), WriteTo, cloning, deep-equals, and conversion; introduced WrappedT<T> helper. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonValueConverter.cs | Replaced token-type switch with instantiation of JsonValueOfJsonPrimitive for primitives. |
| src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs | Replaced inline GUID parsing logic with JsonReaderHelper.TryGetValue call. |
| src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs | Added new overload TryGetValue(ReadOnlySpan<byte>, bool, out Guid) to centralize GUID parsing. |
| src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs | New internal sealed class implementing optimized storage and parsing for JSON primitives. |
| src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs | Switched inline GUID parsing in TryGetValue to JsonReaderHelper.TryGetValue. |
| src/libraries/System.Text.Json/src/System.Text.Json.csproj | Included JsonValueOfJsonPrimitive.cs in project file. |
| Assert.True(JsonNode.DeepEquals(clone, node)); | ||
| } | ||
|
|
||
| private static readonly HashSet<Type> s_convertableTypes = |
There was a problem hiding this comment.
The variable name 's_convertableTypes' contains a spelling error: 'convertable' should be 'convertible'. Consider renaming it to 's_convertibleTypes' for clarity.
Copilot uses AI. Check for mistakes.
|
@eiriktsarpalis I can also take this PR one step further by adding separate derived classes for JSON string and numbers, and introducing the JsonNumber struct like we discussed offline. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Show resolved
Hide resolved
| private readonly JsonValueKind _valueKind; | ||
| private readonly bool _isEscaped; | ||
|
|
||
| public JsonValueOfJsonPrimitive(ref Utf8JsonReader reader, JsonNodeOptions? options) |
There was a problem hiding this comment.
Nit: a ref parameter in a constructor is somewhat unusual, a static factory might be preferable here.
| { | ||
| internal sealed class JsonValueOfJsonPrimitive : JsonValue | ||
| { | ||
| private readonly byte[] _value; |
There was a problem hiding this comment.
Having a byte[] for boolean values is redundant, so to me it seems valuable to keep JsonValue<bool> for that purpose.
Similarly, having a dedicated JsonNumber type that internally defines a discriminated between union between decimal and byte[] for larger numbers sounds like something that might improve performance for the 99.9%.
| return success; | ||
| } | ||
|
|
||
| if (typeof(T) == typeof(decimal) || typeof(T) == typeof(decimal?)) |
There was a problem hiding this comment.
There are limits to how many types we can support with this branching. Assuming the JsonNode encapsulates a JsonSerializerOptions (I don't recall if it does, or can be made to do that) it might be possible to extend conversion support to arbitrary types with simpler logic here.
This reverts commit 30ca4ab.
Adds a new derived class that JsonValueConverter can use when the value is a primitive. Previously
JsonElementOfValuewas being used and more recentlyJsonValue. There are downsides with both. The JsonElement-backed class incurs overhead for theJsonDocumentit needs to create andJsonValuedoes not allow common conversions inTryGet. The new class in this PR solves both by only storing necessary information and allowing conversions. It also preserves original escaping inWriteTo.Fixes #116730