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.

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

EF9: SaveChanges() is significantly slower in .NET9 vs. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping #35239

Open
c5racing opened this issue Nov 29, 2024 · 4 comments · May be fixed by #35326
Assignees

Comments

@c5racing
Copy link

c5racing commented Nov 29, 2024

We use PostgreSQL and are in the process of migrating from .NET8 to .NET9 and in doing so, changing from the Legacy POCO mapping to the newer .ToJson() Mapping.

In the attached sample, there is no data actually being modified. In all cases, dbContext.ChangeTracker.HasChanges(); returns false.

.NET8, version 8.0.11
Legacy Poco Mapping: 0ms
.ToJson(): 300ms

.NET9, version 9.0.0
Legacy Poco Mapping: 0ms
.ToJson(): 2500ms

In both cases, .ToJson() takes longer; however, the difference between .NET8 and .NET9 is 10x more.

The sample references .NET9; however, can be changed to .NET8 for testing. It also relies on Docker for Testcontainers.PostgreSql

https://drive.google.com/file/d/1A_VoN9XgrogMEeevV7mtLh5c2s-Ds2rt/view?usp=drive_link

@c5racing c5racing changed the title EF9: SaveChanges() is much slower when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping EF9: SaveChanges() is much slower in .NET8 vs. .NET9 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping Nov 29, 2024
@c5racing c5racing changed the title EF9: SaveChanges() is much slower in .NET8 vs. .NET9 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping EF9: SaveChanges() is much slower in .NET9 vs. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping Nov 29, 2024
@c5racing c5racing changed the title EF9: SaveChanges() is much slower in .NET9 vs. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping EF9: SaveChanges() is significantly slower in .NET9 vs. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping Nov 29, 2024
@maumar maumar self-assigned this Nov 30, 2024
@roji
Copy link
Member

roji commented Nov 30, 2024

The difference between 8.0 and 9.0 is likely part of the more general #35053 (not specifically related to JSON), though we should test this scenario specifically after the fixes to #35053 to make sure.

Then for the general difference between PG legacy POCO mapping and ToJson, some difference is definitely expected (since legacy POCO treats the whole document as a single property, no change tracking inside); but 300ms does seem quite excessive (any thoughts @ajcvickers?).

@ajcvickers
Copy link
Contributor

Yep, seems slow. But our relational JSON processing code is very inefficient--multiple passes, use of DOM, etc.--so it's not too much of a surprise.

@maumar
Copy link
Contributor

maumar commented Dec 13, 2024

Problem is with List<Guid> property. ListOfValueTypesComparer (as well as ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer) use ValueComparer.ConstructorExpression to build a new instance of valuecomparer every time it's called rather than use the instance passed as an argument (ValueComparer constant is not something we can represent in code for AOT case). This results in us creating 1000s of comparers, which then have their Equal(Guid Guid) delegate lazilly compiled (1000s of times) during save changes.

@maumar
Copy link
Contributor

maumar commented Dec 13, 2024

We should use EqualsExpression SnapshotExpression from the element comparer rather than comparer itself. So, instead of

private static bool Compare(IEnumerable<TElement>? a, IEnumerable<TElement>? b, ValueComparer<TElement> elementComparer)

we should use

private static bool Compare(IEnumerable<TElement>? a, IEnumerable<TElement>? b, Type elementComparerType, Func<TElement, TElement, bool> equalsFunc)

maumar added a commit that referenced this issue Dec 13, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements.
We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer<TElement>) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form).
Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

Fixes #35239
maumar added a commit that referenced this issue Dec 14, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer (presumably to allow support of nested array types in the future), we we can't blindly copy over Equals/Hashcode/Snapshot method lambdas in case types are not matching. For now we check if the both type arguments match - if they do, we copy the funcs, otherwise we fallback to the old behavior, i.e. construct new element comparer so that it can deal with type argument discrepancies. We also disabled generating type mappings for nested collections. Those are not supported by EF, but we were creating mappings (and therefore comparers) for them. Weeding them out simplifies the comparer code.

Fixes #35239
maumar added a commit that referenced this issue Dec 14, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer (presumably to allow support of nested array types in the future), we we can't blindly copy over Equals/Hashcode/Snapshot method lambdas in case types are not matching. For now we check if the both type arguments match - if they do, we copy the funcs, otherwise we fallback to the old behavior, i.e. construct new element comparer so that it can deal with type argument discrepancies. We also disabled generating type mappings for nested collections. Those are not supported by EF, but we were creating mappings (and therefore comparers) for them. Weeding them out simplifies the comparer code.

Fixes #35239
maumar added a commit that referenced this issue Dec 14, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer (presumably to allow support of nested array types in the future), we we can't blindly copy over Equals/Hashcode/Snapshot method lambdas in case types are not matching. For now we check if the both type arguments match - if they do, we copy the funcs, otherwise we fallback to the old behavior, i.e. construct new element comparer so that it can deal with type argument discrepancies. We also disabled generating type mappings for nested collections. Those are not supported by EF, but we were creating mappings (and therefore comparers) for them. Weeding them out simplifies the comparer code.

Fixes #35239
maumar added a commit that referenced this issue Dec 14, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer (presumably to allow support of nested array types in the future), we we can't blindly copy over Equals/Hashcode/Snapshot method lambdas in case types are not matching. For now we check if the both type arguments match - if they do, we copy the funcs, otherwise we fallback to the old behavior, i.e. construct new element comparer so that it can deal with type argument discrepancies.

Fixes #35239
maumar added a commit that referenced this issue Dec 19, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping
maumar added a commit that referenced this issue Dec 19, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer. If we detect this case, we see if the lambdas (Equals, Hashcode, Snapshot) are compatible between what we expect (in the list comparer method) and what we get from the element comparer. If they are compatible we just pass the element lambda to the list method. If they are not compatible we rewrite the element lambda so that they match.

Fixes #35239
maumar added a commit that referenced this issue Dec 19, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer. If we detect this case, we see if the lambdas (Equals, Hashcode, Snapshot) are compatible between what we expect (in the list comparer method) and what we get from the element comparer. If they are compatible we just pass the element lambda to the list method. If they are not compatible we rewrite the element lambda so that they match.

Fixes #35239
maumar added a commit that referenced this issue Dec 19, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer. If we detect this case, we see if the lambdas (Equals, Hashcode, Snapshot) are compatible between what we expect (in the list comparer method) and what we get from the element comparer. If they are compatible we just pass the element lambda to the list method. If they are not compatible we rewrite the element lambda so that they match.

Fixes #35239
maumar added a commit that referenced this issue Dec 19, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer. If we detect this case, we see if the lambdas (Equals, Hashcode, Snapshot) are compatible between what we expect (in the list comparer method) and what we get from the element comparer. If they are compatible we just pass the element lambda to the list method. If they are not compatible we rewrite the element lambda so that they match.

Fixes #35239
maumar added a commit that referenced this issue Dec 20, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping
maumar added a commit that referenced this issue Dec 20, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

For ListOfReferenceTypesComparer it's a bit trickier - TElement of the outer comparer doesn't always match TElement of the element comparer. If we detect this case, we see if the lambdas (Equals, Hashcode, Snapshot) are compatible between what we expect (in the list comparer method) and what we get from the element comparer. If they are compatible we just pass the element lambda to the list method. If they are not compatible we rewrite the element lambda so that they match.

Fixes #35239

refactored into ConvertingValueComparer
maumar added a commit that referenced this issue Dec 21, 2024
…s. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

In order to do that safely we need to make sure that type of the element comparer and the type on the list comparer are compatible (so that when func from element comparer is passed to the list comparer Equals/Hashcode/Snapshot method the resulting expression is valid. We do that by introducing a comparer that converts from one type to another, so that they are always aligned.

Fixes #35239
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants