Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Apply a number of improvements and bugfixes to JsonNode #88194

Merged
merged 4 commits into from
Jul 1, 2023
Merged

Apply a number of improvements and bugfixes to JsonNode #88194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b02f735
Avoid JsonElement torn read in a few JsonArray/JsonDocument and unify…
eiriktsarpalis Jun 28, 2023
d9ac996
Simplify and clean up JsonValue implementation -- fix DeepEquality bugs.
eiriktsarpalis Jun 29, 2023
eabb5b9
Update src/libraries/System.Text.Json/src/System/Text/Json/Nodes/Json…
eiriktsarpalis Jun 29, 2023
02376c5
Avoid delegate allocation in the JsonObject setter.
eiriktsarpalis Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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;
Copy link
Member Author

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.


if (list is null)
{
DetachParent(List[i]);
_jsonElement = null;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
}
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
Open in desktop
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
Loading