Access to context bound variable can incorrectly be optimized out #9825
Comments
|
Investigate for 2.0.1 and maybe re-triage afterwards. |
Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during `OnModelCreating` which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. Solution: To be filled in Resolves #9825
Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during `OnModelCreating` which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. Solution: While evaluating filters, we need to make sure that no complex expression which has context closure variable is partially evaluatable. And when generating filters, for non-queryable member access on context, we need to parametrize it. Resolves #9825
We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during `OnModelCreating` which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. Solution: While evaluating filters, we need to make sure that no complex expression which has context closure variable is partially evaluatable (so it doesn't short-circuit). And when generating filters, for non-queryable member access on context (closure variables but not DbSet), we need to parametrize it. Resolves #9825
…iables in query filters Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during `OnModelCreating` which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. Solution: While evaluating filters, we need to make sure that no complex expression which has context closure variable is partially evaluatable (so it doesn't short-circuit). And when generating filters, for non-queryable member access on context (closure variables but not DbSet), we need to parametrize it. Resolves #9825
Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during `OnModelCreating` which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. Solution: To be filled in Resolves #9825
Replaced short-circuiting optimization in ParameterExtractingExpressionVisitor with cheap null compensation handling. The main advantages to this approach are: 1) No more query cache fragmentation - short circuiting would produce multiple entries in query cache. 2) Less logic/rewriting inside parameter extraction - faster.
Replaced short-circuiting optimization in ParameterExtractingExpressionVisitor with cheap null compensation handling. The main advantages to this approach are: 1) No more query cache fragmentation - short circuiting would produce multiple entries in query cache. 2) Simpler: Less logic/rewriting inside parameter extraction.
|
This patch bug is approved for the 2.0.x patch. Please send a PR to the |
…zed out Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during OnModelCreating which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. The other cause for it is QueryCache has only 1 entry for all the possible values of context bound variables in filter. Solution: Since evaluating filter with context bound variables with different value may not always generate a re-usable query model for all values of variables, whenever we get an expression to evaluate during funcletizing query filter, we replace context, with context parameter (which would be injected later) and return this expression without evaluating anything in-memory. Any exception to be thrown from evaluation of context bound variables will be thrown when actual parameter value will be calculated while running query. Resolves #9825
…zed out Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during OnModelCreating which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. The other cause for it is QueryCache has only 1 entry for all the possible values of context bound variables in filter. Solution: Since evaluating filter with context bound variables with different value may not always generate a re-usable query model for all values of variables, whenever we get an expression to evaluate during funcletizing query filter, we replace context, with context parameter (which would be injected later) and return this expression without evaluating anything in-memory. Any exception to be thrown from evaluation of context bound variables will be thrown when actual parameter value will be calculated while running query. Resolves #9825
…zed out Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during OnModelCreating which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. The other cause for it is QueryCache has only 1 entry for all the possible values of context bound variables in filter. Solution: Since evaluating filter with context bound variables with different value may not always generate a re-usable query model for all values of variables, whenever we get an expression to evaluate during funcletizing query filter, we replace context, with context parameter (which would be injected later) and return this expression without evaluating anything in-memory. Any exception to be thrown from evaluation of context bound variables will be thrown when actual parameter value will be calculated while running query. Resolves #9825
…zed out Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during OnModelCreating which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. The other cause for it is QueryCache has only 1 entry for all the possible values of context bound variables in filter. Solution: Since evaluating filter with context bound variables with different value may not always generate a re-usable query model for all values of variables, whenever we get an expression to evaluate during funcletizing query filter, we replace context, with context parameter (which would be injected later) and return this expression without evaluating anything in-memory. Any exception to be thrown from evaluation of context bound variables will be thrown when actual parameter value will be calculated while running query. Resolves #9825
smitpatel
added
the
closed-fixed
|
Hi, we have a public test feed that you can use to try out the ASP.NET/EF Core 2.0.3 patch! To try out the pre-release patch, please refer to the following guide:
We are looking for feedback on this patch. We'd like to know if you have any issues with this patch by updating your apps and libraries to the latest packages and seeing if it fixes the issues you've had, or if it introduces any new issues. If you have any issues or questions, please reply on this issue to let us know as soon as possible. Thanks, |
divega
changed the title
|
Is that released with 2.0.1? Change logs of 2.0.1 includes this one. |
|
@hikalkan - Yes. |
|
Thanks. This issue is in 2.0.3 milestone which causes the confusion. |
|
@ajcvickers @divega - Can we rename 2.0.3 milestone to 2.0.1 now? |
|
@smitpatel Unlikely, but not really our call. @Eilon? |
|
We decided to have the milestones match the releases, which is 2.0.3. If you click on the milestone you'll see it says 2.0.1 in the description. Perhaps we can make that text a little clearer? |
|
@Eilon I tried to make the description text a bit clearer, but I am not sure how much helps, because it I have not been able find any other place this text is shown, other than when you edit it 😸 Do you known when else it is shown? I know there was a decision taken on this, but haven't some of the original arguments to align to 2.0.3 (e.g. everything was going to be versioned the same, not just the ASP.NET Core meta-package) been invalidated? Some package versions for things like EF Core and MVC are 2.0.1. Releases are 2.0.1 as well: |
|
Well, what I'd much rather us do is just always update everything, e.g. next patch would be 2.0.4 for everything, even if there are no changes. If you can help get buyoff on that from @DamianEdwards , that would simplify many things. |
|
@divega we were never going to version everything the same. The desire to align the milestone names was to ensure they reflected the same time period/release across all repos and make it easier to query all issues in a single milestone across the entire organization. If we do what @Eilon is suggesting, then that will be true for the actual versions too, however, it will mean always shipping every package for every single release, even if we don't change anything, which will have impacts on the runtime store size (as it's cumulative) although that won't matter once we move to the shared framework model. Ultimately, we need to decide on trade-offs between package versioning norms, customer expectations & understanding, build & release management, artifact size, verification complexity, and more. We can chat about it today in our sync. |
|
@DamianEdwards "everything" was an exaggeration, sorry. IIRC, the idea was that .NET Core, ASP.NET Core meta-package and SDK would align, wasn't it? But they don't, which I think is new data. Then there is concrete feedback (although from 1 person right now) in this thread saying that the mapping between the milestone and the package version is confusing, which I don't think is new data... It was obvious it was going to be confusing. Yes, I guess we can chat about this in our sync, again. See you there 😄 |
|
@divega yeah that was the plan, and then the SDK changed to be split out again (for valid reasons), so now we're just back to .NET Core runtime and ASP.NET Core meta-package aligning. We have the OK to split those if need be, but ultimately that doesn't really help anything here. Versioning is hard 😕 |
Definitely :) |
|
@Eilon @divega @DamianEdwards Just another data point on this. In our issue template we ask people which version of EF Core they are using. Thirteen people have reported issues saying they are using 2.0.1. Zero people have reported issues saying that they are using 2.0.3. So it seems from this (limited) data that our customers on GitHub universally think of the release as being 2.0.1 despite efforts to make it appear like it is the 2.0.3 release. |
|
So what release of EF Core resolve this issue? |
|
@antonio-campagnaro It was fixed in the NuGet package versioned 2.0.1, which is part of the ASP.NET meta-package 2.0.3. |
Found investigating #9809. This should work:
modelBuilder .Entity<Report>() .HasQueryFilter(x => !x.IsDeleted && (IsModerated == null || IsModerated == x.IsModerated));but the IsModerated == null part is optimized away.
The text was updated successfully, but these errors were encountered: