Skip to content

Commit

Permalink
Uses existing JSON primitive collection instead of creating a new one
Browse files Browse the repository at this point in the history 
Part of #30677

Builds on #31272

This is the case where the CLR types contains, for example, an `ObservableCollection<int>`. In this case, we populate this existing collection, rather than creating a new instance.

Same still needs to be done for relational materialization of primitive collections.
  • Loading branch information
@ajcvickers
ajcvickers committed Jul 19, 2023
1 parent 6b44512 commit 376263c
Show file tree
Hide file tree
Showing 17 changed files with 2,823 additions and 405 deletions.
79 changes: 72 additions & 7 deletions ...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1517,7 +1517,8 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression)
// - entity construction / property assignments
// - navigation fixups
// - entity instance variable that is returned as end result
var propertyAssignmentReplacer = new ValueBufferTryReadValueMethodsReplacer(propertyAssignmentMap);
var propertyAssignmentReplacer = new ValueBufferTryReadValueMethodsReplacer(
jsonEntityTypeVariable, propertyAssignmentMap);

if (body.Expressions[0] is BinaryExpression
{
Expand Down Expand Up @@ -1866,25 +1867,88 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp

private sealed class ValueBufferTryReadValueMethodsReplacer : ExpressionVisitor
{
private static readonly MethodInfo PopulateListMethod
= typeof(ValueBufferTryReadValueMethodsReplacer).GetMethod(
nameof(PopulateList), BindingFlags.NonPublic | BindingFlags.Static)!;

private readonly Expression _instance;
private readonly Dictionary<IProperty, ParameterExpression> _propertyAssignmentMap;

public ValueBufferTryReadValueMethodsReplacer(Dictionary<IProperty, ParameterExpression> propertyAssignmentMap)
public ValueBufferTryReadValueMethodsReplacer(
Expression instance,
Dictionary<IProperty, ParameterExpression> propertyAssignmentMap)
{
_instance = instance;
_propertyAssignmentMap = propertyAssignmentMap;
}

protected override Expression VisitBinary(BinaryExpression node)
{
if (node.Right is MethodCallExpression methodCallExpression
&& IsPropertyAssignment(methodCallExpression, out var property, out var parameter))
{
if (property!.GetTypeMapping().ElementTypeMapping != null
&& !property.ClrType.IsArray)
{
var currentVariable = Variable(parameter!.Type);
return Block(
new[] { currentVariable },
Assign(
currentVariable,
MakeMemberAccess(_instance, property.GetMemberInfo(forMaterialization: true, forSet: false))),
IfThenElse(
OrElse(
ReferenceEqual(currentVariable, Constant(null)),
ReferenceEqual(parameter, Constant(null))),
MakeBinary(node.NodeType, node.Left, parameter),
Call(
PopulateListMethod.MakeGenericMethod(property.ClrType.TryGetElementType(typeof(IEnumerable<>))!),
parameter,
currentVariable)
));
}

return MakeBinary(node.NodeType, Visit(node.Left), parameter!);
}

return base.VisitBinary(node);
}

protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
=> IsPropertyAssignment(methodCallExpression, out _, out var parameter)
? parameter!
: base.VisitMethodCall(methodCallExpression);

private bool IsPropertyAssignment(
MethodCallExpression methodCallExpression,
out IProperty? property,
out Expression? parameter)
{
if (methodCallExpression.Method.IsGenericMethod
&& methodCallExpression.Method.GetGenericMethodDefinition()
== Infrastructure.ExpressionExtensions.ValueBufferTryReadValueMethod
&& ((ConstantExpression)methodCallExpression.Arguments[2]).Value is IProperty property
&& _propertyAssignmentMap.TryGetValue(property, out var parameter))
&& ((ConstantExpression)methodCallExpression.Arguments[2]).Value is IProperty prop
&& _propertyAssignmentMap.TryGetValue(prop, out var param))
{
return parameter;
property = prop;
parameter = param;
return true;
}

return base.VisitMethodCall(methodCallExpression);
property = null;
parameter = null;
return false;
}

private static IList<T> PopulateList<T>(IList<T> buffer, IList<T> target)
{
target.Clear();
foreach (var value in buffer)
{
target.Add(value);
}

return target;
}
}
}
Expand Down Expand Up @@ -2264,7 +2328,8 @@ private Expression CreateReadJsonPropertyValueExpression(
property.GetJsonValueReaderWriter() ?? property.GetTypeMapping().JsonValueReaderWriter!);

var fromJsonMethod = jsonReaderWriterExpression.Type.GetMethod(
nameof(JsonValueReaderWriter.FromJson), new[] { typeof(Utf8JsonReaderManager).MakeByRefType(), typeof(object) })!;
nameof(JsonValueReaderWriter<object>.FromJsonTyped),
new[] { typeof(Utf8JsonReaderManager).MakeByRefType(), typeof(object) })!;

Expression resultExpression = Convert(
Call(jsonReaderWriterExpression, fromJsonMethod, jsonReaderManagerParameter, Default(typeof(object))),
Expand Down
3 changes: 2 additions & 1 deletion src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ protected override void AppendUpdateColumnValue(
stringBuilder.Append(columnModification.JsonPath);
stringBuilder.Append("', ");

if (columnModification.Property != null)
if (columnModification.Property != null
&& columnModification.Property.GetTypeMapping().ElementTypeMapping == null)
{
base.AppendUpdateColumnValue(updateSqlGeneratorHelper, columnModification, stringBuilder, name, schema);
}
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ protected override void AppendRowsAffectedWhereCondition(StringBuilder commandSt
public override string GenerateNextSequenceValueOperation(string name, string? schema)
=> throw new NotSupportedException(SqliteStrings.SequencesNotSupported);


/// <inheritdoc />
protected override void AppendUpdateColumnValue(
ISqlGenerationHelper updateSqlGeneratorHelper,
Expand All @@ -160,7 +159,8 @@ protected override void AppendUpdateColumnValue(
stringBuilder.Append(columnModification.JsonPath);
stringBuilder.Append("', ");

if (columnModification.Property != null)
if (columnModification.Property != null
&& columnModification.Property.GetTypeMapping().ElementTypeMapping == null)
{
var providerClrType = (columnModification.Property.GetTypeMapping().Converter?.ProviderClrType
?? columnModification.Property.ClrType).UnwrapNullableType();
Expand Down
28 changes: 22 additions & 6 deletions src/EFCore/Storage/Json/JsonNoNullsCollectionReaderWriter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ namespace Microsoft.EntityFrameworkCore.Storage.Json;
/// A <see cref="JsonValueReaderWriter{TValue}" /> for collections of primitive elements that will never be <see langword="null" />.
/// </summary>
/// <typeparam name="TCollection">The collection type.</typeparam>
/// <typeparam name="TConcreteCollection">The collection type to create an index of, if needed.</typeparam>
/// <typeparam name="TElement">The element type.</typeparam>
public class JsonNoNullsCollectionReaderWriter<TCollection, TElement> : JsonValueReaderWriter<IEnumerable<TElement>>
public class JsonNoNullsCollectionReaderWriter<TCollection, TConcreteCollection, TElement> : JsonValueReaderWriter<IEnumerable<TElement?>>
where TCollection : IEnumerable<TElement>
where TConcreteCollection : IList<TElement>
{
private readonly JsonValueReaderWriter<TElement> _elementReaderWriter;

Expand All @@ -26,7 +29,20 @@ public JsonNoNullsCollectionReaderWriter(JsonValueReaderWriter elementReaderWrit
/// <inheritdoc />
public override IEnumerable<TElement> FromJsonTyped(ref Utf8JsonReaderManager manager, object? existingObject = null)
{
var value = new List<TElement>();
IList<TElement> collection;
if (typeof(TCollection).IsArray)
{
collection = new List<TElement>();
}
else if (existingObject == null)
{
collection = Activator.CreateInstance<TConcreteCollection>();
}
else
{
collection = (IList<TElement>)existingObject;
collection.Clear();
}

while (manager.CurrentReader.TokenType != JsonTokenType.EndArray)
{
Expand All @@ -38,21 +54,21 @@ public override IEnumerable<TElement> FromJsonTyped(ref Utf8JsonReaderManager ma
case JsonTokenType.Number:
case JsonTokenType.True:
case JsonTokenType.False:
value.Add(_elementReaderWriter.FromJsonTyped(ref manager));
collection.Add(_elementReaderWriter.FromJsonTyped(ref manager));
break;
}
}

return typeof(TCollection).IsArray ? value.ToArray() : value;
return typeof(TCollection).IsArray ? collection.ToArray() : collection;
}

/// <inheritdoc />
public override void ToJsonTyped(Utf8JsonWriter writer, IEnumerable<TElement> value)
public override void ToJsonTyped(Utf8JsonWriter writer, IEnumerable<TElement?> value)
{
writer.WriteStartArray();
foreach (var element in value)
{
_elementReaderWriter.ToJsonTyped(writer, element);
_elementReaderWriter.ToJsonTyped(writer, element!);
}

writer.WriteEndArray();
Expand Down
27 changes: 21 additions & 6 deletions src/EFCore/Storage/Json/JsonNullRefsCollectionReaderWriter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ namespace Microsoft.EntityFrameworkCore.Storage.Json;
/// A <see cref="JsonValueReaderWriter{TValue}" /> for collections of primitive elements that are a nullable reference type.
/// </summary>
/// <typeparam name="TCollection">The collection type.</typeparam>
/// <typeparam name="TConcreteCollection">The collection type to create an index of, if needed.</typeparam>
/// <typeparam name="TElement">The element type.</typeparam>
public class JsonNullRefsCollectionReaderWriter<TCollection, TElement> : JsonValueReaderWriter<IEnumerable<TElement?>>
public class JsonNullRefsCollectionReaderWriter<TCollection, TConcreteCollection, TElement> : JsonValueReaderWriter<IEnumerable<TElement?>>
where TElement : class
where TCollection : IEnumerable<TElement?>
where TConcreteCollection : IList<TElement?>
{
private readonly JsonValueReaderWriter<TElement> _elementReaderWriter;

Expand All @@ -27,8 +30,20 @@ public JsonNullRefsCollectionReaderWriter(JsonValueReaderWriter<TElement> elemen
/// <inheritdoc />
public override IEnumerable<TElement?> FromJsonTyped(ref Utf8JsonReaderManager manager, object? existingObject = null)
{
// TODO: Existing collection on entity instance will be passed in.
var value = new List<TElement?>();
IList<TElement?> collection;
if (typeof(TCollection).IsArray)
{
collection = new List<TElement?>();
}
else if (existingObject == null)
{
collection = Activator.CreateInstance<TConcreteCollection>();
}
else
{
collection = (IList<TElement?>)existingObject;
collection.Clear();
}

while (manager.CurrentReader.TokenType != JsonTokenType.EndArray)
{
Expand All @@ -40,15 +55,15 @@ public JsonNullRefsCollectionReaderWriter(JsonValueReaderWriter<TElement> elemen
case JsonTokenType.Number:
case JsonTokenType.True:
case JsonTokenType.False:
value.Add(_elementReaderWriter.FromJsonTyped(ref manager));
collection.Add(_elementReaderWriter.FromJsonTyped(ref manager));
break;
case JsonTokenType.Null:
value.Add(null);
collection.Add(null);
break;
}
}

return typeof(TCollection).IsArray ? value.ToArray() : value;
return typeof(TCollection).IsArray ? collection.ToArray() : collection;
}

/// <inheritdoc />
Expand Down
27 changes: 22 additions & 5 deletions src/EFCore/Storage/Json/JsonNullStructsCollectionReaderWriter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@ namespace Microsoft.EntityFrameworkCore.Storage.Json;
/// A <see cref="JsonValueReaderWriter{TValue}" /> for collections of primitive elements that are a nullable value type.
/// </summary>
/// <typeparam name="TCollection">The collection type.</typeparam>
/// <typeparam name="TConcreteCollection">The collection type to create an index of, if needed.</typeparam>
/// <typeparam name="TElement">The element type.</typeparam>
public class JsonNullStructsCollectionReaderWriter<TCollection, TElement> : JsonValueReaderWriter<IEnumerable<TElement?>>
public class
JsonNullStructsCollectionReaderWriter<TCollection, TConcreteCollection, TElement> : JsonValueReaderWriter<IEnumerable<TElement?>>
where TElement : struct
where TCollection : IEnumerable<TElement?>
where TConcreteCollection : IList<TElement?>
{
private readonly JsonValueReaderWriter<TElement> _elementReaderWriter;

Expand All @@ -27,7 +31,20 @@ public JsonNullStructsCollectionReaderWriter(JsonValueReaderWriter<TElement> ele
/// <inheritdoc />
public override IEnumerable<TElement?> FromJsonTyped(ref Utf8JsonReaderManager manager, object? existingObject = null)
{
var value = new List<TElement?>();
IList<TElement?> collection;
if (typeof(TCollection).IsArray)
{
collection = new List<TElement?>();
}
else if (existingObject == null)
{
collection = Activator.CreateInstance<TConcreteCollection>();
}
else
{
collection = (IList<TElement?>)existingObject;
collection.Clear();
}

while (manager.CurrentReader.TokenType != JsonTokenType.EndArray)
{
Expand All @@ -39,15 +56,15 @@ public JsonNullStructsCollectionReaderWriter(JsonValueReaderWriter<TElement> ele
case JsonTokenType.Number:
case JsonTokenType.True:
case JsonTokenType.False:
value.Add(_elementReaderWriter.FromJsonTyped(ref manager));
collection.Add(_elementReaderWriter.FromJsonTyped(ref manager));
break;
case JsonTokenType.Null:
value.Add(null);
collection.Add(null);
break;
}
}

return typeof(TCollection).IsArray ? value.ToArray() : value;
return typeof(TCollection).IsArray ? collection.ToArray() : collection;
}

/// <inheritdoc />
Expand Down
31 changes: 27 additions & 4 deletions src/EFCore/Storage/TypeMappingSourceBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,17 +166,40 @@ protected virtual bool TryFindMappingForPrimitiveCollection(
typeof(JsonCastValueReaderWriter<>).MakeGenericType(elementType.UnwrapNullableType()), elementReader)!;
}

var typeToInstantiate = FindTypeToInstantiate();

collectionReaderWriter = mappingInfo.JsonValueReaderWriter
?? (JsonValueReaderWriter?)Activator.CreateInstance(
(elementType.IsNullableValueType()
? typeof(JsonNullStructsCollectionReaderWriter<,>)
? typeof(JsonNullStructsCollectionReaderWriter<,,>)
: elementType.IsValueType
? typeof(JsonNoNullsCollectionReaderWriter<,>)
: typeof(JsonNullRefsCollectionReaderWriter<,>))
.MakeGenericType(modelClrType, elementType.UnwrapNullableType()),
? typeof(JsonNoNullsCollectionReaderWriter<,,>)
: typeof(JsonNullRefsCollectionReaderWriter<,,>))
.MakeGenericType(modelClrType, typeToInstantiate, elementType.UnwrapNullableType()),
elementReader);

return true;

Type FindTypeToInstantiate()
{
if (modelClrType.IsArray)
{
return modelClrType;
}

if (!modelClrType.IsAbstract)
{
var constructor = modelClrType.GetDeclaredConstructor(null);
if (constructor?.IsPublic == true)
{
return modelClrType;
}
}

var listOfT = typeof(List<>).MakeGenericType(elementType);

return modelClrType.IsAssignableFrom(listOfT) ? listOfT : modelClrType;
}
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Query/JsonQueryFixtureBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,8 @@ private static void AssertOwnedRoot(JsonOwnedRoot expected, JsonOwnedRoot actual
{
Assert.Equal(expected.Name, actual.Name);
Assert.Equal(expected.Number, actual.Number);
Assert.Equal(expected.Names, actual.Names);
Assert.Equal(expected.Numbers, actual.Numbers);

AssertOwnedBranch(expected.OwnedReferenceBranch, actual.OwnedReferenceBranch);
Assert.Equal(expected.OwnedCollectionBranch.Count, actual.OwnedCollectionBranch.Count);
Expand All @@ -339,6 +341,8 @@ private static void AssertOwnedBranch(JsonOwnedBranch expected, JsonOwnedBranch
Assert.Equal(expected.Fraction, actual.Fraction);
Assert.Equal(expected.Enum, actual.Enum);
Assert.Equal(expected.NullableEnum, actual.NullableEnum);
Assert.Equal(expected.Enums, actual.Enums);
Assert.Equal(expected.NullableEnums, actual.NullableEnums);

AssertOwnedLeaf(expected.OwnedReferenceLeaf, actual.OwnedReferenceLeaf);
Assert.Equal(expected.OwnedCollectionLeaf.Count, actual.OwnedCollectionLeaf.Count);
Expand Down
Loading

0 comments on commit 376263c

Please sign in to comment.