Fix performance degradation in converters because the instance was not reused

[cincuranet]

Fixes #36856.

[cincuranet]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndriySvyryd]

Do you plan on porting this to GA?

[cincuranet]

Yes: #36898.

[roji]

As discussed offline, we can add a DEBUG check to ensure that the shaper tree never contains a NewExpression for a ValueConverter - this would be the regression test for this. This shouldn't be too hard - we already had some validation in https://github.com/roji/efcore/blob/eb5f3905d3141dfb74dd67daed828285e483f166/src/EFCore/Query/LiftableConstantProcessor.cs#L263 (even though that's currently disabled).

See also #33517 which is referenced from that commented-out code and which is relevant to everything that's going on here (i.e. why value converters are problematic).

[cincuranet]

The innerShaper (where this comes from) does not go through (at the moment) LiftableConstantProcessor. I guess we can probably plug visitor at a different spot. Is it worth it?

[roji]

Are you sure that's the case? That in itself would mean that JSON doesn't work with precompiled queries, no?

I thought when you dump the final shaper (at the point where we also do liftable constant inlining/lifting), you can always recursively see all the shapers of all structural types in the tree, otherwise I'm a bit confused about how it's all working.

[cincuranet]

Are you sure that's the case?

As far as I can tell.

That in itself would mean that JSON doesn't work with precompiled queries, no?

🤷‍♂️

I thought when you dump the final shaper (at the point where we also do liftable constant inlining/lifting), you can always recursively see all the shapers of all structural types in the tree, otherwise I'm a bit confused about how it's all working.

Sadly for you, I'm not an expert in this area.

[roji]

Well, I did a quick minimal test program that queries something that has a JSON complex type in it (full source below):

_ = await context.Blogs.ToListAsync();

I put a breakpoint right before the liftable constant processor, and dumped the shaper with ExpressionPrinter.Print. The full dump is below, but the relevant part with the innerShaper is the following:

ShaperProcessingExpressionVisitor.IncludeJsonEntityReference<Blog, BlogDetails>(
    queryContext: queryContext, 
    keyPropertyValues: null, 
    jsonReaderData: jsonReader, 
    structuralType: instance1, 
    innerShaper: (queryContext, namelessParameter{3}, namelessParameter{4}) => 
    {
        BlogDetails entityShaperMaterializer;
        return entityShaperMaterializer = 
        {
            MaterializationContext materializationContext2;
            IComplexType entityType2;
            BlogDetails instance2;
...

So there's a call to ShaperProcessingExpressionVisitor.IncludeJsonEntityReference (which is a regular .NET method, not code-generated or anything), and the innerShaper is passed in as an argument, but it's present in the shaper tree as an expression (the parameter type is Func<QueryContext, object[]?, JsonReaderData, TRelatedStructural>, but then in the shaper expression tree the innerShaper is an Expression<Func<...>>.

I think that means that the liftable constant processor recurses inside the innerShaper just like any other expression tree node - let me know if I'm missing something.

Full code

await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

_ = await context.Blogs.ToListAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>().ComplexProperty(b => b.Details, b => b.ToJson());
    }
}

public class Blog
{
    public int Id { get; set; }

    public BlogDetails Details { get; set; }
}

public class BlogDetails
{
    public string Foo { get; set; }
    public int Bar { get; set; }
}

Full shaper dump

queryContext => SingleQueryingEnumerable.Create<Blog>(
    relationalQueryContext: (RelationalQueryContext)queryContext, 
    relationalCommandResolver: parameters => [LIFTABLE Constant: RelationalCommandCache.QueryExpression(
        Projection Mapping:
            EmptyProjectionMember -> Dictionary<IPropertyBase, int> { [Property: Blog.Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd, 0], [ComplexProperty: Blog.Details (BlogDetails) Required, 1] }
        SELECT b.Id, b.Details -> 
        FROM Blogs AS b) | Resolver: c => new RelationalCommandCache(
        c.Dependencies.MemoryCache, 
        c.RelationalDependencies.QuerySqlGeneratorFactory, 
        c.RelationalDependencies.RelationalParameterBasedSqlProcessorFactory, 
        Projection Mapping:
            EmptyProjectionMember -> Dictionary<IPropertyBase, int> { [Property: Blog.Id (int) Required PK AfterSave:Throw ValueGenerated.OnAdd, 0], [ComplexProperty: Blog.Details (BlogDetails) Required, 1] }
        SELECT b.Id, b.Details -> 
        FROM Blogs AS b, 
        False, 
        MultipleParameters
    )].GetRelationalCommandTemplate(parameters), 
    readerColumns: null, 
    shaper: (queryContext, dataReader, resultContext, resultCoordinator) => 
    {
        Blog entity;
        Stream jsonStream;
        JsonReaderData jsonReader;
        Utf8JsonReaderManager jsonReaderManager;
        jsonStream = dataReader.IsDBNull(1) ? default(MemoryStream) : SqlServerStructuralJsonTypeMapping.CreateUtf8Stream(dataReader.GetString(1));
        jsonReader = jsonStream == default(MemoryStream) ? default(JsonReaderData) : new JsonReaderData(jsonStream);
        jsonReader != default(JsonReaderData) ? 
        {
            jsonReaderManager = new Utf8JsonReaderManager(
                jsonReader, 
                queryContext.QueryLogger
            );
            jsonReaderManager.MoveNext();
            jsonReaderManager.CaptureState();
        } : default(void);
        entity = 
        {
            MaterializationContext materializationContext1;
            IEntityType entityType1;
            Blog instance1;
            InternalEntityEntry entry1;
            bool hasNullKey1;
            materializationContext1 = new MaterializationContext(
                [LIFTABLE Constant: ValueBuffer | Resolver: _ => (object)ValueBuffer.Empty], 
                queryContext.Context
            );
            instance1 = default(Blog);
            entry1 = queryContext.TryGetEntry(
                key: [LIFTABLE Constant: Key: Blog.Id PK | Resolver: c => c.Dependencies.Model.FindEntityType("Blog").FindPrimaryKey()], 
                keyValues: new object[]{ (object)dataReader.GetInt32(0) }, 
                throwOnNullKey: True, 
                hasNullKey: hasNullKey1);
            !(hasNullKey1) ? entry1 != default(InternalEntityEntry) ? 
            {
                entityType1 = entry1.EntityType;
                return instance1 = (Blog)entry1.Entity;
            } : 
            {
                ISnapshot shadowSnapshot1;
                shadowSnapshot1 = [LIFTABLE Constant: Snapshot | Resolver: _ => Snapshot.Empty];
                entityType1 = [LIFTABLE Constant: EntityType: Blog | Resolver: namelessParameter{0} => namelessParameter{0}.Dependencies.Model.FindEntityType("Blog")];
                instance1 = switch (entityType1)
                {
                    case [LIFTABLE Constant: EntityType: Blog | Resolver: namelessParameter{1} => namelessParameter{1}.Dependencies.Model.FindEntityType("Blog")]: 
                        {
                            return 
                            {
                                Blog instance;
                                instance = new Blog();
                                instance.<Id>k__BackingField = dataReader.GetInt32(0);
                                (instance is IInjectableService) ? ((IInjectableService)instance).Injected(
                                    context: materializationContext1.Context, 
                                    entity: instance, 
                                    queryTrackingBehavior: TrackAll, 
                                    structuralType: [LIFTABLE Constant: EntityType: Blog | Resolver: namelessParameter{2} => namelessParameter{2}.Dependencies.Model.FindEntityType("Blog")]) : default(void);
                                return instance;
                            }}
                    default: 
                        default(Blog)
                }
                ;
                ShaperProcessingExpressionVisitor.IncludeJsonEntityReference<Blog, BlogDetails>(
                    queryContext: queryContext, 
                    keyPropertyValues: null, 
                    jsonReaderData: jsonReader, 
                    structuralType: instance1, 
                    innerShaper: (queryContext, namelessParameter{3}, namelessParameter{4}) => 
                    {
                        BlogDetails entityShaperMaterializer;
                        return entityShaperMaterializer = 
                        {
                            MaterializationContext materializationContext2;
                            IComplexType entityType2;
                            BlogDetails instance2;
                            materializationContext2 = new MaterializationContext(
                                [LIFTABLE Constant: ValueBuffer | Resolver: _ => (object)ValueBuffer.Empty], 
                                queryContext.Context
                            );
                            instance2 = default(BlogDetails);
                            {
                                ISnapshot shadowSnapshot2;
                                shadowSnapshot2 = [LIFTABLE Constant: Snapshot | Resolver: _ => Snapshot.Empty];
                                entityType2 = [LIFTABLE Constant: ComplexType: Blog.Details#BlogDetails (BlogDetails) | Resolver: namelessParameter{5} => namelessParameter{5}.Dependencies.Model.FindEntityType("Blog").FindComplexProperty("Details").ComplexType];
                                instance2 = 
                                {
                                    Utf8JsonReaderManager jsonReaderManager;
                                    JsonTokenType tokenType;
                                    BlogDetails instance;
                                    int namelessParameter{6};
                                    string namelessParameter{7};
                                    jsonReaderManager = new Utf8JsonReaderManager(
                                        namelessParameter{4}, 
                                        queryContext.QueryLogger
                                    );
                                    tokenType = jsonReaderManager.CurrentReader.TokenType;
                                    Loop(Break: done Continue: )
                                    {
                                        {
                                            tokenType = jsonReaderManager.MoveNext();
                                            switch (tokenType)
                                            {
                                                case PropertyName: 
                                                    jsonReaderManager.CurrentReader.ValueTextEquals((ReadOnlySpan<byte>)Encoding.UTF8.GetBytes("Bar")
                                                        .AsSpan()) ? 
                                                    {
                                                        jsonReaderManager.MoveNext();
                                                        namelessParameter{6} = (int)[LIFTABLE Constant: JsonInt32ReaderWriter | Resolver: c => c.Dependencies.Model.FindEntityType("Blog").FindComplexProperty("Details").ComplexType.FindProperty("Bar").GetJsonValueReaderWriter() ?? c.Dependencies.Model.FindEntityType("Blog").FindComplexProperty("Details").ComplexType.FindProperty("Bar").GetTypeMapping().JsonValueReaderWriter].FromJsonTyped(
                                                            manager: jsonReaderManager, 
                                                            existingObject: default(object));
                                                    } : jsonReaderManager.CurrentReader.ValueTextEquals((ReadOnlySpan<byte>)Encoding.UTF8.GetBytes("Foo")
                                                        .AsSpan()) ? 
                                                    {
                                                        jsonReaderManager.MoveNext();
                                                        namelessParameter{7} = (string)[LIFTABLE Constant: JsonStringReaderWriter | Resolver: c => c.Dependencies.Model.FindEntityType("Blog").FindComplexProperty("Details").ComplexType.FindProperty("Foo").GetJsonValueReaderWriter() ?? c.Dependencies.Model.FindEntityType("Blog").FindComplexProperty("Details").ComplexType.FindProperty("Foo").GetTypeMapping().JsonValueReaderWriter].FromJsonTyped(
                                                            manager: jsonReaderManager, 
                                                            existingObject: default(object));
                                                    } : default(void)
                                                case EndObject: 
                                                    Goto(break done)
                                                default: 
                                                    {
                                                        jsonReaderManager.Skip();
                                                    }
                                            }
                                        }}
                                    jsonReaderManager.CaptureState();
                                    instance = new BlogDetails();
                                    instance.<Bar>k__BackingField = namelessParameter{6};
                                    instance.<Foo>k__BackingField = namelessParameter{7};
                                    return instance;
                                };
                                return instance2;
                            }return instance2;
                        };
                    }, 
                    fixup: (namelessParameter{8}, namelessParameter{9}) => 
                    {
                        return namelessParameter{8}.<Details>k__BackingField = namelessParameter{9};
                    }, 
                    performFixup: True);
                entry1 = entityType1 == default(IEntityType) ? default(InternalEntityEntry) : queryContext.StartTracking(
                    entityType: entityType1, 
                    entity: instance1, 
                    snapshot: shadowSnapshot1);
                return instance1;
            } : default(void);
            return instance1;
        };
        return entity;
    }, 
    contextType: BlogContext, 
    standAloneStateManager: False, 
    detailedErrorsEnabled: False, 
    threadSafetyChecksEnabled: True)

[cincuranet]

I did override VisitNew in LiftableConstantProcessor and printed all types there. This the result.

Microsoft.EntityFrameworkCore.Storage.MaterializationContext
Parent
Microsoft.EntityFrameworkCore.Storage.MaterializationContext
Microsoft.EntityFrameworkCore.ChangeTracking.Internal.Snapshot`1[[System.String, System.Private.CoreLib, Version=10.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]
Child

The innerShaper I'm talking about comes from this.

Did you have something else in mind?

[roji]

OK, let's not spend more time on this.