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

7x/field resolver performance #3668

Merged
merged 2 commits into from
Apr 16, 2019
Merged

7x/field resolver performance #3668

merged 2 commits into from
Apr 16, 2019

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Apr 15, 2019

This brings some performance enhancements to the way we resolve Field, Fields and PropertyName when these are instantiated using an Expression. E.g if you do q.Term<Project>(p=>p.Name, "value")

We were visiting the expression multiple times

  1. To create a comparison value we used a specialized visitor
  2. This visitor calls tostring which in turns calls the BCL tostring visitor
  3. Then we did some regex replace to create a sane key
  4. Visit expression and check if there are compilable parts and if so make sure we do not cache the expression
  5. Finally resolve the expression using the connection settings using another visitor

This PR

Combines the comparison vistor/tostring visitor/caching visitor into one. We no longer rely on Expression.ToString() and since we only need to deal with a certain type of expression our tostring performance better then the BCL one.

Then if its we pass it off the FieldExpressionVisitor if not in local cache already. While writing this I wonder if FieldExpressionVisitor could reuse the stack from our new ToStringVisitor. Will follow up in a comment.

The benchmark compares:

  1. Expression<Func<T, object>>.ToString() as Boxed Expression
  2. Expression<Func<T, TValue>>.ToString() as Unboxed Expression
  3. FieldInferrer.Resolve(Expression<Func<T, object>>) as Field Resolver
  4. FieldInferrer.Resolve(Expression<Func<T, TValue>>) as Field Resolver Unboxed , only available on last benchmark

before

Method Jit Runtime Mean Error StdDev Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
'Boxed Expression' LegacyJit Clr 5.622 ns 2.8143 ns 0.1543 ns 0.0004 - - -
'Unboxed Expression' LegacyJit Clr 2.970 ns 1.6041 ns 0.0879 ns 0.0003 - - -
'Field Resolver' LegacyJit Clr 9.877 ns 3.5268 ns 0.1933 ns 0.0004 - - -
'Boxed Expression' RyuJit Clr 5.638 ns 1.6133 ns 0.0884 ns 0.0004 - - -
'Unboxed Expression' RyuJit Clr 2.994 ns 1.3333 ns 0.0731 ns 0.0003 - - -
'Field Resolver' RyuJit Clr 9.668 ns 2.2694 ns 0.1244 ns 0.0004 - - -
'Boxed Expression' RyuJit Core 5.097 ns 1.2232 ns 0.0670 ns 0.0003 - - 2 B
'Unboxed Expression' RyuJit Core 3.496 ns 2.1039 ns 0.1153 ns 0.0003 - - 2 B
'Field Resolver' RyuJit Core 10.640 ns 0.3072 ns 0.0168 ns 0.0004 - - 2 B

after

Method Jit Runtime Mean Error StdDev Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
'Boxed Expression' LegacyJit Clr 6.140 ns 0.2625 ns 0.0144 ns 0.0004 - - -
'Unboxed Expression' LegacyJit Clr 3.275 ns 0.9076 ns 0.0497 ns 0.0003 - - -
'Field Resolver' LegacyJit Clr 4.938 ns 6.8149 ns 0.3735 ns 0.0003 - - -
'Field Resolver Unboxed' LegacyJit Clr 3.777 ns 1.1002 ns 0.0603 ns 0.0003 - - -
'Boxed Expression' RyuJit Clr 6.201 ns 2.6049 ns 0.1428 ns 0.0004 - - -
'Unboxed Expression' RyuJit Clr 3.491 ns 6.1798 ns 0.3387 ns 0.0003 - - -
'Field Resolver' RyuJit Clr 5.049 ns 2.4973 ns 0.1369 ns 0.0003 - - -
'Field Resolver Unboxed' RyuJit Clr 3.810 ns 5.3521 ns 0.2934 ns 0.0003 - - -
'Boxed Expression' RyuJit Core 5.981 ns 7.2205 ns 0.3958 ns 0.0003 - - 2 B
'Unboxed Expression' RyuJit Core 7.120 ns 58.5053 ns 3.2069 ns 0.0003 - - 2 B
'Field Resolver' RyuJit Core 6.504 ns 9.8342 ns 0.5390 ns 0.0003 - - 2 B
'Field Resolver Unboxed' RyuJit Core 5.464 ns 9.9388 ns 0.5448 ns 0.0003 - - 2 B

@Mpdreamz Mpdreamz requested a review from russcam April 15, 2019 12:38
@Mpdreamz
Copy link
Member Author

Tried locally to reuse the visit of the ToStringVisitor and it creating a second stack for the fieldname to resolve so it doesn't need to visit the expression as well.

Don't see any real benefit and it adds quite some complexity so resetting locally.

Benchmark for reference

Method Jit Runtime Mean Error StdDev Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
'Boxed Expression' LegacyJit Clr 5.914 ns 7.5240 ns 0.4124 ns 0.0004 - - -
'Unboxed Expression' LegacyJit Clr 3.445 ns 3.2429 ns 0.1778 ns 0.0003 - - -
'Field Resolver' LegacyJit Clr 5.022 ns 1.4263 ns 0.0782 ns 0.0004 - - -
'Field Resolver Unboxed' LegacyJit Clr 3.657 ns 4.1828 ns 0.2293 ns 0.0003 - - -
'Boxed Expression' RyuJit Clr 6.113 ns 0.0929 ns 0.0051 ns 0.0004 - - -
'Unboxed Expression' RyuJit Clr 3.317 ns 0.1476 ns 0.0081 ns 0.0003 - - -
'Field Resolver' RyuJit Clr 6.347 ns 31.5901 ns 1.7316 ns 0.0004 - - -
'Field Resolver Unboxed' RyuJit Clr 3.754 ns 0.0714 ns 0.0039 ns 0.0003 - - -
'Boxed Expression' RyuJit Core 5.315 ns 0.9369 ns 0.0514 ns 0.0003 - - 2 B
'Unboxed Expression' RyuJit Core 3.855 ns 7.7585 ns 0.4253 ns 0.0003 - - 2 B
'Field Resolver' RyuJit Core 6.263 ns 0.8671 ns 0.0475 ns 0.0004 - - 2 B
'Field Resolver Unboxed' RyuJit Core 4.779 ns 0.2023 ns 0.0111 ns 0.0003 - - 2 B

@Mpdreamz Mpdreamz merged commit 0fb1149 into 7.x Apr 16, 2019
@Mpdreamz Mpdreamz deleted the 7x/field-resolver-performance branch April 16, 2019 11:46
@Mpdreamz
Copy link
Member Author

Reviewed this morning during our sync meeting so merging myself

Mpdreamz added a commit that referenced this pull request Apr 16, 2019
* Use custom tostring visitor to create _comparisonValue on Field

* Combine variable visitor with new tostring visitor

(cherry picked from commit 0fb1149)
@Mpdreamz
Copy link
Member Author

cherry-picked to master

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.

1 participant