Skip to content

Extend IAccessOptimizer to support getting/setting single property value #1944

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

Merged
merged 8 commits into from
Jan 1, 2019

Conversation

maca88
Copy link
Contributor

@maca88 maca88 commented Dec 21, 2018

This is an alternative approach (#1942) to avoid reflection when UseReflectionOptimizer enabled. Here I was forced to use IL as IOptimizableSetter exposes only an Emit method. I think that this approach is better, but is also more invasive and contains some ugly code to avoid a breaking change.

@maca88
Copy link
Contributor Author

maca88 commented Dec 23, 2018

Here are some performance numbers of the added [Get\Set]PropertyValue methods:

IGetter.Get total time for 100000 iterations: 131ms
IAccessOptimizer.GetPropertyValue total time for 100000 iterations: 37ms
IAccessOptimizer.GetPropertyValues total time for 100000 iterations: 16ms

ISetter.Set total time for 100000 iterations: 242ms
IAccessOptimizer.SetPropertyValue total time for 100000 iterations: 49ms
IAccessOptimizer.SetPropertyValues total time for 100000 iterations: 23ms

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it is a better approach than #1942.

@@ -163,10 +163,10 @@ private static void EmitCastToReference(ILGenerator il, System.Type type)

private GetPropertyValueInvoker GenerateGetPropertyValueMethod(IGetter getter)
{
if (getter == null)
return null;
Copy link
Member

@fredericDelaporte fredericDelaporte Dec 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot the null case... Now that we do no more yield just null when the argument is not optimizable, the case where it is null has to be handled separately, because the null coalesce operator does not work on method groups. (And the ternary requires some explicit casting.)

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