Fix performance degradation in converters because the instance was not reused#36895
Fix performance degradation in converters because the instance was not reused#36895cincuranet merged 1 commit intodotnet:mainfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AndriySvyryd
left a comment
AndriySvyryd
left a comment
There was a problem hiding this comment.
Do you plan on porting this to GA?
cincuranet
force-pushed
the
converter-new-fix
branch
from
d4cabc0 to
7821ac5
Compare
|
Yes: #36898. |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
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
force-pushed
the
converter-new-fix
branch
from
7821ac5 to
d55fd8f
Compare
|
The |
|
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. |
As far as I can tell.
🤷♂️
Sadly for you, I'm not an expert in this area. |
|
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 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 codeawait 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 dumpqueryContext => 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) |
|
I did override The Did you have something else in mind? |
|
OK, let's not spend more time on this. |
3 participants
Fixes #36856.