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

Query/Perf: don't use Invoke in value comparer ObjectEqualsExpression lambdas #35206

Closed
maumar opened this issue Nov 25, 2024 · 1 comment
Closed
Assignees
Labels
area-perf area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression
Milestone

Comments

@maumar
Copy link
Contributor

maumar commented Nov 25, 2024

Example comparer lambda we generate in EF9 for PopulateIncludeCollection method:

parentIdentifierValueComparers: [LIFTABLE Constant: Func<object, object, bool>[] { Func<object, object, bool>, Func<object, object, bool> } | Resolver: _ => new Func<object, object, bool>[]
{ 
    (left, right) => left == null ? right == null : right != null && Invoke((v1, v2) => v1 == v2, (int)left, (int)right), 
    (left, right) => left == null ? right == null : right != null && Invoke((v1, v2) => v1 == v2, (int)left, (int)right) 
}], 

Instead we should inline the inner lambda like so:

parentIdentifierValueComparers: [LIFTABLE Constant: Func<object, object, bool>[] { Func<object, object, bool>, Func<object, object, bool> } | Resolver: _ => new Func<object, object, bool>[]
{ 
    (left, right) => left == null ? right == null : right != null && (int)left == (int)right, 
    (left, right) => left == null ? right == null : right != null && (int)left == (int)right 
}], 

This change yields significant improvement for Include collection scenarios with large number of elements:

ef 9 before the Invoke fix

Method Async Mean Error StdDev Op/s Gen0 Gen1 Allocated
PredicateMultipleIncludes False 322.6 ms 0.97 ms 0.86 ms 3.099 13000.0000 6000.0000 79.48 MB
PredicateMultipleIncludes True 344.9 ms 6.79 ms 6.67 ms 2.899 14000.0000 7000.0000 87.72 MB

ef 9 after the invoke fix

Method Async Mean Error StdDev Op/s Gen0 Gen1 Allocated
PredicateMultipleIncludes False 242.8 ms 2.39 ms 2.12 ms 4.119 8000.0000 5000.0000 51.69 MB
PredicateMultipleIncludes True 263.4 ms 2.21 ms 2.06 ms 3.797 10000.0000 9000.0000 59.93 MB
@maumar maumar added this to the 9.0.x milestone Nov 25, 2024
@maumar maumar changed the title Query/Perf: don't use Invoke in value comparer lambdas when constructing shaper with PopulateCollection Query/Perf: don't use Invoke in value comparer ObjectEqualsExpression lambdas Nov 25, 2024
maumar added a commit that referenced this issue Nov 25, 2024
…s when constructing shaper with PopulateCollection

In EF9 we changed the way we generate shapers in preparation for AOT scenarios. We no longer can embed arbitrary objects into the shaper, instead we need to provide a way to construct that object in code, or simulate the functionality it used to provide.
One of the examples was use of ValueComparers in PopulateIncludeCollection. Now instead of passing list of ValueComparer objects to use (which we can't reliably generate in code), we pass the delegate which is used to compare two values:

```
(left, right) => left == null ? right == null : right != null && Invoke((v1, v2) => v1 == v2, (int)left, (int)right)
```

This incurs a performance hit on some scenarios with collections, but can be improved by simplifying the delegate we use. Instead of having nested lambdas and using Invoke, we can inline the body of the nested lambda directly into the outer lambda, like so:

```
(left, right) => left == null ? right == null : right != null && (int)left == (int)right
```

This one change yields significant improvement in the affected scenarios (reducing both time spent and allocations):

ef 9 before the Invoke fix

|                    Method | Async |     Mean |   Error |  StdDev |  Op/s |       Gen0 |      Gen1 | Allocated |
|-------------------------- |------ |---------:|--------:|--------:|------:|-----------:|----------:|----------:|
| PredicateMultipleIncludes | False | 322.6 ms | 0.97 ms | 0.86 ms | 3.099 | 13000.0000 | 6000.0000 |  79.48 MB |
| PredicateMultipleIncludes |  True | 344.9 ms | 6.79 ms | 6.67 ms | 2.899 | 14000.0000 | 7000.0000 |  87.72 MB |

ef 9 after the invoke fix

|                    Method | Async |     Mean |   Error |  StdDev |  Op/s |       Gen0 |      Gen1 | Allocated |
|-------------------------- |------ |---------:|--------:|--------:|------:|-----------:|----------:|----------:|
| PredicateMultipleIncludes | False | 242.8 ms | 2.39 ms | 2.12 ms | 4.119 |  8000.0000 | 5000.0000 |  51.69 MB |
| PredicateMultipleIncludes |  True | 263.4 ms | 2.21 ms | 2.06 ms | 3.797 | 10000.0000 | 9000.0000 |  59.93 MB |
@maumar maumar self-assigned this Nov 26, 2024
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 26, 2024
maumar added a commit that referenced this issue Nov 27, 2024
…s when constructing shaper with PopulateCollection (#35207)

In EF9 we changed the way we generate shapers in preparation for AOT scenarios. We no longer can embed arbitrary objects into the shaper, instead we need to provide a way to construct that object in code, or simulate the functionality it used to provide.
One of the examples was use of ValueComparers in PopulateIncludeCollection. Now instead of passing list of ValueComparer objects to use (which we can't reliably generate in code), we pass the delegate which is used to compare two values:

```
(left, right) => left == null ? right == null : right != null && Invoke((v1, v2) => v1 == v2, (int)left, (int)right)
```

This incurs a performance hit on some scenarios with collections, but can be improved by simplifying the delegate we use. Instead of having nested lambdas and using Invoke, we can inline the body of the nested lambda directly into the outer lambda, like so:

```
(left, right) => left == null ? right == null : right != null && (int)left == (int)right
```

This one change yields significant improvement in the affected scenarios (reducing both time spent and allocations):

ef 9 before the Invoke fix

|                    Method | Async |     Mean |   Error |  StdDev |  Op/s |       Gen0 |      Gen1 | Allocated |
|-------------------------- |------ |---------:|--------:|--------:|------:|-----------:|----------:|----------:|
| PredicateMultipleIncludes | False | 322.6 ms | 0.97 ms | 0.86 ms | 3.099 | 13000.0000 | 6000.0000 |  79.48 MB |
| PredicateMultipleIncludes |  True | 344.9 ms | 6.79 ms | 6.67 ms | 2.899 | 14000.0000 | 7000.0000 |  87.72 MB |

ef 9 after the invoke fix

|                    Method | Async |     Mean |   Error |  StdDev |  Op/s |       Gen0 |      Gen1 | Allocated |
|-------------------------- |------ |---------:|--------:|--------:|------:|-----------:|----------:|----------:|
| PredicateMultipleIncludes | False | 242.8 ms | 2.39 ms | 2.12 ms | 4.119 |  8000.0000 | 5000.0000 |  51.69 MB |
| PredicateMultipleIncludes |  True | 263.4 ms | 2.21 ms | 2.06 ms | 3.797 | 10000.0000 | 9000.0000 |  59.93 MB |
@maumar
Copy link
Contributor Author

maumar commented Nov 30, 2024

fixed in 9.0.1 - 1a69d23

@maumar maumar closed this as completed Nov 30, 2024
@maumar maumar modified the milestones: 9.0.x, 9.0.1 Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression
Projects
None yet
Development

No branches or pull requests

1 participant