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

[release/9.0-staging] Fix to #35206 - Query/Perf: don't use Invoke in value comparer lambdas when constructing shaper with PopulateCollection #35207

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Nov 25, 2024

Fixes #35206
Port of #35128

Description

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

Customer impact

Queries using collection navigation with significant amount of data suffer large performance degradation when compared with EF8

How found
Multiple customer reports on 9.0.0

Regression
Yes, from 8.0. (perf regression only, no functional regression)

Testing
Ad-hoc perf testing with BenchmarkDotNet. Functional change already covered by numerous tests.

Risk
Low. Inlining a nested lambda (rather than invoke) is a common and safe optimization. Quirk added, just in case.

…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 changed the title Fix to #35206 - Query/Perf: don't use Invoke in value comparer lambdas when constructing shaper with PopulateCollection [release/9.0-staging] Fix to #35206 - Query/Perf: don't use Invoke in value comparer lambdas when constructing shaper with PopulateCollection Nov 25, 2024
@maumar maumar requested a review from SamMonoRT November 25, 2024 23:30
@maumar
Copy link
Contributor Author

maumar commented Nov 26, 2024

This change doesn't have significant impact on AOT scenario (100 runs of 50 queries each with warmup)

before the fix
Benchmark 1: .\AdventureworksAotEF9.exe
Time (mean ± σ): 818.8 ms ± 10.9 ms [User: 402.0 ms, System: 42.2 ms]
Range (min … max): 803.9 ms … 864.1 ms 100 runs

after the fix
Benchmark 1: .\AdventureworksAotEF9.exe
Time (mean ± σ): 819.3 ms ± 14.0 ms [User: 385.0 ms, System: 44.6 ms]
Range (min … max): 806.7 ms … 906.6 ms 100 runs

@maumar maumar added this to the 9.0.x milestone Nov 26, 2024
@maumar maumar merged commit 1a69d23 into release/9.0-staging Nov 27, 2024
7 checks passed
@maumar maumar deleted the fix35206_90 branch November 27, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants