Skip to content

Commit

Permalink
Apply a number of improvements and bugfixes to JsonNode (#88194)
Browse files Browse the repository at this point in the history 
* Avoid JsonElement torn read in a few JsonArray/JsonDocument and unify code patterns.

* Simplify and clean up JsonValue implementation -- fix DeepEquality bugs.

* Update src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfTPrimitive.cs

* Avoid delegate allocation in the JsonObject setter.
  • Loading branch information
@eiriktsarpalis
eiriktsarpalis committed Jul 1, 2023
1 parent d388585 commit 1db4357
Show file tree
Hide file tree
Showing 26 changed files with 513 additions and 657 deletions.
4 changes: 2 additions & 2 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,9 @@ The System.Text.Json library is built-in as part of the shared framework in .NET
<Compile Include="System\Text\Json\Nodes\JsonObject.IDictionary.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValue.CreateOverloads.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValue.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValueNotTrimmable.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValueOfTCustomized.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValueOfT.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValueTrimmable.cs" />
<Compile Include="System\Text\Json\Nodes\JsonValueOfTPrimitive.cs" />
<Compile Include="System\Text\Json\Reader\ConsumeNumberResult.cs" />
<Compile Include="System\Text\Json\Reader\ConsumeTokenResult.cs" />
<Compile Include="System\Text\Json\Reader\JsonReaderException.cs" />
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ internal int GetEndIndex(int index, bool includeEndElement)

internal ReadOnlyMemory<byte> GetRootRawValue()
{
return GetRawValue(0, includeQuotes : true);
return GetRawValue(0, includeQuotes: true);
}

internal ReadOnlyMemory<byte> GetRawValue(int index, bool includeQuotes)
Expand Down
18 changes: 6 additions & 12 deletions src/libraries/System.Text.Json/src/System/Text/Json/JsonPropertyDictionary.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,7 @@ public void CopyTo(KeyValuePair<string, T>[] array, int index)
}
}

public IEnumerator<KeyValuePair<string, T>> GetEnumerator()
{
foreach (KeyValuePair<string, T> item in _propertyList)
{
yield return item;
}
}
public List<KeyValuePair<string, T>>.Enumerator GetEnumerator() => _propertyList.GetEnumerator();

public IList<string> Keys => GetKeyCollection();

Expand Down Expand Up @@ -205,11 +199,11 @@ public T? this[string propertyName]

set
{
SetValue(propertyName, value);
SetValue(propertyName, value, out bool _);
}
}

public T? SetValue(string propertyName, T value, Action? assignParent = null)
public T? SetValue(string propertyName, T value, out bool valueAlreadyInDictionary)
{
if (IsReadOnly)
{
Expand All @@ -223,14 +217,14 @@ public T? this[string propertyName]

CreateDictionaryIfThresholdMet();

valueAlreadyInDictionary = false;
T? existing = null;

if (_propertyDictionary != null)
{
// Fast path if item doesn't exist in dictionary.
if (_propertyDictionary.TryAdd(propertyName, value))
{
assignParent?.Invoke();
_propertyList.Add(new KeyValuePair<string, T>(propertyName, value));
return null;
}
Expand All @@ -239,6 +233,7 @@ public T? this[string propertyName]
if (ReferenceEquals(existing, value))
{
// Ignore if the same value.
valueAlreadyInDictionary = true;
return null;
}
}
Expand All @@ -256,18 +251,17 @@ public T? this[string propertyName]
if (ReferenceEquals(current.Value, value))
{
// Ignore if the same value.
valueAlreadyInDictionary = true;
return null;
}

existing = current.Value;
}

assignParent?.Invoke();
_propertyList[i] = new KeyValuePair<string, T>(propertyName, value);
}
else
{
assignParent?.Invoke();
_propertyDictionary?.Add(propertyName, value);
_propertyList.Add(new KeyValuePair<string, T>(propertyName, value));
Debug.Assert(existing == null);
Expand Down
15 changes: 12 additions & 3 deletions src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.IList.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

if (list is null)
{
DetachParent(List[i]);
_jsonElement = null;
}
else
{
for (int i = 0; i < list.Count; i++)
{
DetachParent(list[i]);
}

List.Clear();
list.Clear();
}
}

/// <summary>
Expand Down
165 changes: 77 additions & 88 deletions src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,45 +48,41 @@ public JsonArray(params JsonNode?[] items) : base()
InitializeFromArray(items);
}

internal override JsonNode InternalDeepClone()
internal override JsonValueKind GetValueKindCore() => JsonValueKind.Array;

internal override JsonNode DeepCloneCore()
{
if (_jsonElement.HasValue)
GetUnderlyingRepresentation(out List<JsonNode?>? list, out JsonElement? jsonElement);

if (list is null)
{
return new JsonArray(_jsonElement.Value.Clone(), Options);
return jsonElement.HasValue
? new JsonArray(jsonElement.Value.Clone(), Options)
: new JsonArray(Options);
}

List<JsonNode?> list = List;

var jsonArray = new JsonArray(Options)
{
_list = new List<JsonNode?>(list.Count)
};

for (int i = 0; i < list.Count; i++)
{
JsonNode? item = list[i];
if (item is null)
{
jsonArray.Add(null);
}
else
{
jsonArray.Add(item.DeepClone());
}
jsonArray.Add(list[i]?.DeepCloneCore());
}

return jsonArray;
}

internal override bool DeepEquals(JsonNode? node)
internal override bool DeepEqualsCore(JsonNode? node)
{
switch (node)
{
case null or JsonObject:
return false;
case JsonValue value:
// JsonValueTrimmable/NonTrimmable can hold the array type so calling this method to continue the deep comparision.
return value.DeepEquals(this);
// JsonValue instances have special comparison semantics, dispatch to their implementation.
return value.DeepEqualsCore(this);
case JsonArray array:
List<JsonNode?> currentList = List;
List<JsonNode?> otherList = array.List;
Expand Down Expand Up @@ -153,17 +149,12 @@ private void InitializeFromArray(JsonNode?[] items)
/// </exception>
public static JsonArray? Create(JsonElement element, JsonNodeOptions? options = null)
{
if (element.ValueKind == JsonValueKind.Null)
{
return null;
}

if (element.ValueKind == JsonValueKind.Array)
return element.ValueKind switch
{
return new JsonArray(element, options);
}

throw new InvalidOperationException(SR.Format(SR.NodeElementWrongType, nameof(JsonValueKind.Array)));
JsonValueKind.Null => null,
JsonValueKind.Array => new JsonArray(element, options),
_ => throw new InvalidOperationException(SR.Format(SR.NodeElementWrongType, nameof(JsonValueKind.Array))),
};
}

internal JsonArray(JsonElement element, JsonNodeOptions? options = null) : base(options)
Expand All @@ -183,28 +174,20 @@ internal JsonArray(JsonElement element, JsonNodeOptions? options = null) : base(
[RequiresDynamicCode(JsonValue.CreateDynamicCodeMessage)]
public void Add<T>(T? value)
{
if (value == null)
JsonNode? nodeToAdd = value switch
{
Add(null);
}
else
{
JsonNode jNode = value as JsonNode ?? new JsonValueNotTrimmable<T>(value);
null => null,
JsonNode node => node,
_ => JsonValue.Create(value, Options)
};

// Call the IList.Add() implementation.
Add(jNode);
}
Add(nodeToAdd);
}

internal List<JsonNode?> List
{
get
{
CreateNodes();
Debug.Assert(_list != null);
return _list;
}
}
/// <summary>
/// Gets or creates the underlying list containing the element nodes of the array.
/// </summary>
internal List<JsonNode?> List => _list is { } list ? list : InitializeList();

internal JsonNode? GetItem(int index)
{
Expand Down Expand Up @@ -237,72 +220,78 @@ public override void WriteTo(Utf8JsonWriter writer, JsonSerializerOptions? optio
ThrowHelper.ThrowArgumentNullException(nameof(writer));
}

if (_jsonElement.HasValue)
GetUnderlyingRepresentation(out List<JsonNode?>? list, out JsonElement? jsonElement);

if (list is null && jsonElement.HasValue)
{
_jsonElement.Value.WriteTo(writer);
jsonElement.Value.WriteTo(writer);
}
else
{
CreateNodes();
Debug.Assert(_list != null);

options ??= s_defaultOptions;

writer.WriteStartArray();

for (int i = 0; i < _list.Count; i++)
foreach (JsonNode? element in List)
{
JsonNodeConverter.Instance.Write(writer, _list[i]!, options);
if (element is null)
{
writer.WriteNullValue();
}
else
{
element.WriteTo(writer, options);
}
}

writer.WriteEndArray();
}
}

private void CreateNodes()
private List<JsonNode?> InitializeList()
{
if (_list is null)
{
CreateNodesCore();
}
GetUnderlyingRepresentation(out List<JsonNode?>? list, out JsonElement? jsonElement);

void CreateNodesCore()
if (list is null)
{
// Even though _list initialization can be subject to races,
// ensure that contending threads use a coherent view of jsonElement.
if (jsonElement.HasValue)
{
JsonElement jElement = jsonElement.Value;
Debug.Assert(jElement.ValueKind == JsonValueKind.Array);

// Because JsonElement cannot be read atomically there might be torn reads,
// however the order of read/write operations guarantees that that's only
// possible if the value of _list is non-null.
JsonElement? jsonElement = _jsonElement;
Interlocked.MemoryBarrier();
List<JsonNode?>? list = _list;
list = new List<JsonNode?>(jElement.GetArrayLength());

if (list is null)
foreach (JsonElement element in jElement.EnumerateArray())
{
JsonNode? node = JsonNodeConverter.Create(element, Options);
node?.AssignParent(this);
list.Add(node);
}
}
else
{
list = new();
}

if (jsonElement.HasValue)
{
JsonElement jElement = jsonElement.Value;
Debug.Assert(jElement.ValueKind == JsonValueKind.Array);

list = new List<JsonNode?>(jElement.GetArrayLength());
// Ensure _jsonElement is written to after _list
_list = list;
Interlocked.MemoryBarrier();
_jsonElement = null;
}

foreach (JsonElement element in jElement.EnumerateArray())
{
JsonNode? node = JsonNodeConverter.Create(element, Options);
node?.AssignParent(this);
list.Add(node);
}
}
return list;
}

// Ensure _jsonElement is written to after _list
_list = list;
Interlocked.MemoryBarrier();
_jsonElement = null;
}
}
/// <summary>
/// Provides a coherent view of the underlying representation of the current node.
/// The jsonElement value should be consumed if and only if the list value is null.
/// </summary>
private void GetUnderlyingRepresentation(out List<JsonNode?>? list, out JsonElement? jsonElement)
{
// Because JsonElement cannot be read atomically there might be torn reads,
// however the order of read/write operations guarantees that that's only
// possible if the value of _list is non-null.
jsonElement = _jsonElement;
Interlocked.MemoryBarrier();
list = _list;
}

[ExcludeFromCodeCoverage] // Justification = "Design-time"
Expand Down
Loading

0 comments on commit 1db4357

Please sign in to comment.