Fix to #35212 - Query/Perf: Compile identifier lambdas passed to PopulateIncludeCollection rather than inline #35213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lateIncludeCollection rather than inline PopulateIncludeCollection (as well as couple other methods) take a number of delegate arguments. For scenarios with significant number of entities we see significant perf improvement when these delegates are compiled (like we used to do in EF8), rather than inlined. Fixes #35212
ajcvickers
approved these changes
Nov 26, 2024
maumar
added a commit
that referenced
this pull request
Nov 26, 2024
…lateIncludeCollection rather than inline Fixes #35212 Port of #35213 **Description** In EF9 we changed the way we generate shapers in preparation for AOT scenarios. As part of these changes we started inlining some delegates passed to PopulateIncludeCollection (as well as couple other methods), rather than compiling them. For scenarios with large number of entities we see significant perf degradation when these delegates are inlined, as opposed to compiled (like we used to do in EF8). This change reverts to EF8 behavior. **Customer impact** Queries using collection navigation with significant amount of data suffer large performance degradation when compared with EF8. No good workaround. **How found** Multiple customer reports on 9.0.0. **Regression** Yes, from 8.0. Perf regression only, no functional regression here. **Testing** Ad-hoc perf testing with BenchmarkDotNet. Functional change already covered by numerous tests. **Risk** Low - essentially reverting back to EF8 behavior, quirk added. **Benchmarks** before the fix (but already with invoke fix and no interpretation) | Method | Async | Mean | Error | StdDev | Op/s | Gen0 | Gen1 | Allocated | |------------ |------ |---------:|--------:|---------:|------:|-----------:|----------:|----------:| | MultiInclue | False | 455.1 ms | 8.94 ms | 10.29 ms | 2.197 | 11000.0000 | 6000.0000 | 67.92 MB | | MultiInclue | True | 435.4 ms | 1.77 ms | 1.66 ms | 2.297 | 11000.0000 | 6000.0000 | 67.92 MB | after the fix: | Method | Async | Mean | Error | StdDev | Op/s | Gen0 | Gen1 | Allocated | |------------ |------ |---------:|--------:|--------:|------:|----------:|----------:|----------:| | MultiInclue | False | 363.3 ms | 6.72 ms | 6.29 ms | 2.752 | 9000.0000 | 3000.0000 | 58.51 MB | | MultiInclue | True | 351.9 ms | 2.08 ms | 1.74 ms | 2.842 | 9000.0000 | 3000.0000 | 58.51 MB | This gets us pretty close to the EF8 numbers which were: | Method | Async | Mean | Error | StdDev | Op/s | Gen0 | Gen1 | Allocated | |------------ |------ |---------:|--------:|--------:|------:|----------:|----------:|----------:| | MultiInclue | False | 297.1 ms | 1.47 ms | 1.30 ms | 3.365 | 8000.0000 | 6000.0000 | 52.4 MB | | MultiInclue | True | 290.2 ms | 3.76 ms | 3.52 ms | 3.446 | 8500.0000 | 6000.0000 | 52.4 MB |
maumar
added a commit
that referenced
this pull request
Nov 27, 2024
…lateIncludeCollection rather than inline (#35217) Fixes #35212 Port of #35213 **Description** In EF9 we changed the way we generate shapers in preparation for AOT scenarios. As part of these changes we started inlining some delegates passed to PopulateIncludeCollection (as well as couple other methods), rather than compiling them. For scenarios with large number of entities we see significant perf degradation when these delegates are inlined, as opposed to compiled (like we used to do in EF8). This change reverts to EF8 behavior. **Customer impact** Queries using collection navigation with significant amount of data suffer large performance degradation when compared with EF8. No good workaround. **How found** Multiple customer reports on 9.0.0. **Regression** Yes, from 8.0. Perf regression only, no functional regression here. **Testing** Ad-hoc perf testing with BenchmarkDotNet. Functional change already covered by numerous tests. **Risk** Low - essentially reverting back to EF8 behavior, quirk added. **Benchmarks** before the fix (but already with invoke fix and no interpretation) | Method | Async | Mean | Error | StdDev | Op/s | Gen0 | Gen1 | Allocated | |------------ |------ |---------:|--------:|---------:|------:|-----------:|----------:|----------:| | MultiInclue | False | 455.1 ms | 8.94 ms | 10.29 ms | 2.197 | 11000.0000 | 6000.0000 | 67.92 MB | | MultiInclue | True | 435.4 ms | 1.77 ms | 1.66 ms | 2.297 | 11000.0000 | 6000.0000 | 67.92 MB | after the fix: | Method | Async | Mean | Error | StdDev | Op/s | Gen0 | Gen1 | Allocated | |------------ |------ |---------:|--------:|--------:|------:|----------:|----------:|----------:| | MultiInclue | False | 363.3 ms | 6.72 ms | 6.29 ms | 2.752 | 9000.0000 | 3000.0000 | 58.51 MB | | MultiInclue | True | 351.9 ms | 2.08 ms | 1.74 ms | 2.842 | 9000.0000 | 3000.0000 | 58.51 MB | This gets us pretty close to the EF8 numbers which were: | Method | Async | Mean | Error | StdDev | Op/s | Gen0 | Gen1 | Allocated | |------------ |------ |---------:|--------:|--------:|------:|----------:|----------:|----------:| | MultiInclue | False | 297.1 ms | 1.47 ms | 1.30 ms | 3.365 | 8000.0000 | 6000.0000 | 52.4 MB | | MultiInclue | True | 290.2 ms | 3.76 ms | 3.52 ms | 3.446 | 8500.0000 | 6000.0000 | 52.4 MB |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PopulateIncludeCollection (as well as couple other methods) take a number of delegate arguments. For scenarios with significant number of entities we see significant perf improvement when these delegates are compiled (like we used to do in EF8), rather than inlined.
Fixes #35212