-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Apply a number of improvements and bugfixes to JsonNode #88194
Conversation
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsThis PR applies a number of improvements and bugfixes to JsonNode following up from #87381.
|
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfTPrimitive.cs
Outdated
Show resolved
Hide resolved
…ValueOfTPrimitive.cs
@@ -31,12 +31,21 @@ public void Add(JsonNode? item) | |||
/// </summary> | |||
public void Clear() | |||
{ | |||
for (int i = 0; i < List.Count; i++) | |||
List<JsonNode?>? list = _list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates the code so that List
is not accidentally initialized here.
@@ -5,22 +5,14 @@ namespace System.Text.Json.Nodes | |||
{ | |||
public abstract partial class JsonNode | |||
{ | |||
// trimming-safe default JsonSerializerOptions instance used by JsonNode methods. | |||
private protected static readonly JsonSerializerOptions s_defaultOptions = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer necessary since the JsonNode.WriteTo
implementations no longer compose via intermediate JsonConverter.Write
calls that require a JsonSerializerOptions
.
InitializeIfRequired(); | ||
Debug.Assert(_dictionary != null); | ||
_dictionary.Add(propertyName, value); | ||
Dictionary.Add(propertyName, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaces initialization code with a Dictionary
property that drives initialization, following the style of the JsonArray
implementation.
@@ -220,16 +216,19 @@ internal override void GetPath(List<string> path, JsonNode? child) | |||
|
|||
internal void SetItem(string propertyName, JsonNode? value) | |||
{ | |||
InitializeIfRequired(); | |||
Debug.Assert(_dictionary != null); | |||
JsonNode? existing = _dictionary.SetValue(propertyName, value, () => value?.AssignParent(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to avoid a delegate allocation in the setter method.
{ | ||
if (_jsonElement.HasValue) | ||
GetUnderlyingRepresentation(out JsonPropertyDictionary<JsonNode?>? dictionary, out JsonElement? jsonElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to avoid potential races with the intializer method that clears out the _jsonElement
field.
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.IList.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonNode.To.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs
Show resolved
Hide resolved
Sharing updated binary size numbers from the Store just for tracking:
Very small 20 KB regression (I suspect just generally for the new stuff added in #87381, especially I guess those new |
You mean |
Yeah that's the one. I was thinking that might be another case of MCG being overzealous and preemptively rooting all generated CCWs for |
Some more updated numbers, looks like the latest few PRs did help a little bit again 😄
So that's another small (but free) ~60 KB improvement. Note that the baseline with the previous version doesn't match that from #88194 (comment), but that's only because I've rebased the dogfooding branch on top of our main in the meantime. Just consider the relative size delta between different System.Text.Json versions 🙂 |
This PR applies a number of improvements and bugfixes to JsonNode following up from #87381.
JsonElement
field directly inJsonObject
andJsonArray
, related to the fixes found in Ensure delayed initialization of JsonArray/JsonObject is thread-safe #77567. Makes adjustments so that code in the two classes is more uniform. Change contained in b02f735.JsonValue
, more specifically this removes the obsoleteJsonValueTrimmable
/JsonValueNotTrimmable
distinction in favor of a common resolver-agnostic implementation. Fixes a number of issues related to the newly addedDeepEquals
method when comparingJsonValue
. Change contained in d9ac996.JsonNode
serialization implementations.cc @RaymondHuy