Performance: Adds a LINQ optimization for member access in LINQ-to-SQL #2924
Conversation
notheotherben
requested review from
j82w,
khdang,
kirankumarkolli and
sboshra
as code owners
There was a problem hiding this comment.
Overall comment:
Is the underlying problem here isolated to Constants? I think this change could help in the scenario that is currently your focus - but is there anything more architecturally that could/should be changed to accommodate a large class of issues?
notheotherben
requested review from
ealsur,
FabianMeiswinkel,
kirillg and
neildsh
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@khdang and @sboshra and @timsander1 please review this PR. |
j82w
changed the title
| @@ -42,14 +43,59 @@ protected override Expression VisitMemberInit(MemberInitExpression node) | |||
| return node; | |||
| } | |||
|
|
|||
| private Expression EvaluateMemberAccess(Expression expression) | |||
There was a problem hiding this comment.
can you please add functional test to cover this function so the target scenarios of this change are clear?
There was a problem hiding this comment.
Hi @khdang, I can certainly add some further tests which exercise this code path, however they'll be identical to the existing test suite.
This code change is responsible for handling any .x.y.z property accesses, which would previously have required the compilation and invocation of a delegate. Given that ambient variables are captured in an implicit structure, this applies to any query which attempts to pattern match to an ambient variable.
As such, this acts as a transparent performance optimization for this (very) common use case without any external behavioural changes. Paired with the fact that the list of expressions which are evaluated is pre-filtered, and I suspect that it should be very rare indeed that this code path is not followed (which is a good thing given the performance and concurrency benefits).
…n a LINQ-to-SQL expression This optimization handles the common case of accessing a member (variable, field, property) in the ambient scope, as shown below: ``` var bob = "Bob"; users.Where(u => u.Username == bob); ``` In this scenario, we will attempt to avoid compiling a lambda expression to evaluate `bob` and instead rely on reflection member access to retrieve the constant value. This helps us avoid the global lock held by `System.Reflection.Emit.DynamicMethod` while constructing the dynamic method table, which alleviates a serious performance bottleneck on highly threaded systems. In particular, this helps avoid a pathological async-over-sync situation which can lead to thread pool exhaustion and hot-lock thrashing.
…reflection property access perf
…on evaluator can be triggered or bypassed
notheotherben
force-pushed
the
feature/linq-sql-member-access
branch
from
2d3ef47 to
78ff617
Compare
|
Thanks @notheotherben for doing this optimization! |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Thanks for getting this merged folks, I suspect this should have a pretty sizeable benefit for our user-base. |
Pull Request Template
Description
This optimization handles the common case of accessing a member (variable, field, property) in the ambient scope, as shown below:
In this scenario, we will attempt to avoid compiling a lambda expression to evaluate
boband instead rely on reflection member access to retrieve the constant value. This helps us avoid the global lock held bySystem.Reflection.Emit.DynamicMethodwhile constructing the dynamic method table, which alleviates a serious performance bottleneck on highly threaded systems. In particular, this helps avoid a pathological async-over-sync situation which can lead to thread pool exhaustion and hot-lock thrashing.The goal of this change is to provide teams who face this problem with a reasonable means of optimizing their query code (use variables which hold pre-computed constants to avoid delegate compilations), instead of forcing them to rewrite their codebases to use stored SQL, or to implement some form of SQL caching.
Type of change