NH-3952: removing usages of NHibernate.Linq.EnumerableHelper #557

fredericDelaporte · 2017-02-24T18:26:13Z

NHibernate.Linq.EnumerableHelper has following comment:

// TODO rename / remove - reflection helper above is better

Done with this pull request.
It includes some additional test cases for classes impacted by this change which were not already thoroughly tested by existing test cases.
An explicit test for choosing which reflection implementation to choose from the other patterns found in NHibernate has been added too. (Unsurprisingly, caching as a static variable the thread safe MethodInfo prove to be the fastest by a wide margin. As it does not look to me as an added code complexity, pattern used in this pull request on each class affected by the removal of EnumerableHelper.)

Tracked under Jira task NH-3952.

EnumerableHelper is not removed but just marked as Obsolete, since it is publicly exposed and may be directly referenced by NHibernate users.

…eflection pattern to stick with this refactoring.

hazzik · 2017-02-24T20:11:57Z

There is slightly more performant version than "Cached method definition + make gen" exists.

        public static MethodInfo GetMethod2<T, TResult>(Func<T, TResult> func, T arg1)
        {
            return func.Method;
        }

The idea that you extract a method from a bare "Method group delegate".

fredericDelaporte · 2017-02-24T22:30:43Z

Thanks for the suggestion,

The case "cached method definition + make gen" should not be used if we can simply use a "cached method". I have done so in this pull request commits.
But when generic type parameter actual type varies at runtime, we cannot use directly a "cached method", and I think we cannot use the GetMethod2<T, TResult> alone either. We would have with it to get the definition and make generic at runtime too.

Now, actually ReflectionHelper use another technique for getting a MethodInfo, like:

public static MethodInfo GetMethod<TSource>(Expression<Action<TSource>> method)
{
	return ((MethodCallExpression)method.Body).Method;
}

Should we change this in favor of your GetMethod2?

About the quite "blunt" performance test on the other current patterns I have found in Nhibernate.Linq implementation, here is a sample result from my laptop, including your suggested method (they can vary considerably, but the ranking remains the same):

Blunt perf timings:
Cached method: 00:00:00.0000031
Cached method definition + make gen: 00:00:00.0017098
Hazzik GetMethod: 00:00:00.0013696
Hazzik GetMethodDefinition + make gen: 00:00:00.0047166
ReflectionHelper.GetMethod: 00:00:00.0029977
ReflectionHelper.GetMethodDefinition + make gen: 00:00:00.0052582
EnumerableHelper.GetMethod(generic overload): 00:00:00.0120367

So your method looks actually better than the one in place in ReflectionHelper.

Currently, on code I have modified, I use in order of priority whenever possible:

Cached method (initialized with ReflectionHelper.GetMethod; for method, generic or not, for which we know the argument types at compile time)
Cached method definition + make gen (initialized with ReflectionHelper.GetMethodDefinition; for generic method for which we do not know all the argument types at compile time)
Cached method + overload search for non-generic method for which we do not know all the argument types at compile time. (NH-3850 only, WIP with commits which may get rebased.)

And that is all.

Replacing GetMethod/GetMethodDefinition by your GetMethod2/GetMethodDefinition2 would impact more than 200 calls. It should no be hard to do, but maybe would be a "too big change" for perf optimization done in the wild (without actually measuring the overall gain on some complete use case like declaring & executing a query, first time & with query plan cache hit).

I may instead introduce those two methods on ReflectionHelper, add some code comment asking for favoring them, and use them in the files this pull request impacts.

What do you think of this please?

fredericDelaporte · 2017-02-27T12:45:39Z

And well, my point on "performances" was more on choosing to which pattern already in use I should stick for cleaning up usages of EnumerableHelper as the existing comment was suggesting, than introducing yet a new one. Maybe should we do that with another task.
(By the way, the pattern in use with ReflectionHelper is slightly slower than yours, but has the advantage of not allowing at compile time to try using a code block as a 'function', while yours will fails at runtime with a code block. When runtime performances are not critical, I tend to favor robustness.)

hazzik · 2017-02-28T00:32:31Z

src/NHibernate/Linq/EnumerableHelper.cs

-
-	// TODO rename / remove - reflection helper above is better
+	
+	[Obsolete("ReflectionHelper is better")]


Please change to [Obsolete("Please use ReflectionHelper instead")]

hazzik · 2017-02-28T00:33:12Z

src/NHibernate/Linq/NestedSelects/NestedSelectRewriter.cs

@@ -16,7 +16,18 @@ namespace NHibernate.Linq.NestedSelects
 {
 	static class NestedSelectRewriter
 	{
-		private static readonly MethodInfo CastMethod = EnumerableHelper.GetMethod("Cast", new[] { typeof (IEnumerable) }, new[] { typeof (object[]) });
+		private static readonly MethodInfo CastMethod = ReflectionHelper.GetMethod(


I would love these fields to be concentrated in EnumerableHelper class (as internal static readonly fields), or other class, if we are making EnumerableHelper obsolete.

I have to check if a sound naming convention would do. With overloads and methods having the same name but belonging to other types, putting them all on ReflectionHelper could end up a bit messy.
Maybe should we use some ReflectionHelper internal subclass for each commonly reflected type to hold their cached method definition. I will test that soon.

fredericDelaporte · 2017-02-28T18:11:39Z

The Obsolete reason is rewritten.

I have added a ReflectionCache class along ReflectionHelper, holding a subclass EnumerableMethods with MethodInfo fields. All those additions are internal.
Currently there is only one subclass, but I expect more to come, by example if we do the same caching for Queryable methods used in ProcessFirst, ProcessSingle, ExpressionParameterVisitor.

To avoid ambiguity with overloads, I have put in place a naming convention. It is not really simple. Here is how I have documented it in code:

// When adding a method to this cache, please follow the naming convention of those subclasses and fields:
//  - Add your method to a subclass named according to the type holding the method, and suffixed with "Methods".
//  - Name the field according to the method name.
//  - If the method has overloads, suffix it with "With" followed by its parameter names. Do not list parameters
//    common to all overloads.
//  - If the method is a generic method definition, add "Definition" as final suffix.
//  - If the method is generic, suffix it with "On" followed by its generic parameter type names.
// Avoid caching here narrow cases, such as those using specific types and unlikely to be used by many classes.
// Cache them instead in classes using them.

I have considered the NestedSelectRewriter reflection cases depending on type NHibernate.Linq.NestedSelects.Tuple to be too narrow for this method cache. So they stay defined in this NestedSelectRewriter class.
Same for CreateQueryMethodDefinition in DefaultQueryProvider. And I have not considered moving cached MethodInfofound in classes outside of the NHibernate.Linq namespace, such as those in ProxyFactory.

The above naming will not be enough for disambiguation of overloads having same parameter names but different parameter types without being generic. We should probably use type names instead of parameter names for those cases. I still favor parameter names when possible because I find it more readable and generally more concise than type names. (Imagine type name of a resultSelector...)

hazzik · 2017-02-28T19:40:50Z

src/NHibernate/Linq/NestedSelects/NestedSelectRewriter.cs

-			var selectMethod = EnumerableHelper.GetMethod("Select",
-														   new[] { typeof (IEnumerable<>), typeof (Func<,>) },
-														   new[] { typeof (Tuple), elementType });
+			var selectMethod = SelectMethodDefinition.MakeGenericMethod(new[] { typeof(Tuple), elementType });


Shouldn't this use ReflectionCache.EnumerableMethods.SelectMethodDefinition?

Indeed, missed it.

hazzik · 2017-02-28T19:46:00Z

src/NHibernate/Linq/EnumerableHelper.cs

+
+			internal static readonly MethodInfo CastDefinition =
+				ReflectionHelper.GetMethodDefinition(() => Enumerable.Cast<object>(null));
+			internal static readonly MethodInfo CastOnObjectArray =


I think this needs to be moved back to NestedSelectRewriter

Was unsure about that one, agreed.

hazzik · 2017-02-28T19:46:48Z

src/NHibernate/Linq/EnumerableHelper.cs

@@ -94,7 +93,46 @@ internal static System.Type GetPropertyOrFieldType(this MemberInfo memberInfo)
 		}
 	}

-	// TODO rename / remove - reflection helper above is better
+	internal static class ReflectionCache


Please move this to a separate class in NHibernate.Util namespace.

Way more cleaner.

fredericDelaporte · 2017-02-28T21:03:26Z

Changes done. Should I rebase for merging commits/cleaning up history? (I prefer to wait for review rather than doing it at once, as this would not help reviewer to review only last changes.)

hazzik · 2017-03-01T01:48:56Z

Should I rebase for merging commits/cleaning up history?

Yes, please squash all your changes down to one commit.

hazzik · 2017-03-01T03:02:56Z

Don't worry, I squashed myself.

fredericDelaporte · 2017-03-01T08:43:36Z

Ok, thanks.

fredericDelaporte added 4 commits February 24, 2017 02:00

NH-3952: non-regression test cases

0d634f5

NH-3952: NestedSelectRewriter currated.

b79fff9

NH-3952: Added a quick&dirty performance test for deciding to which r…

c385ebe

…eflection pattern to stick with this refactoring.

NH-3952: usages of EnumerableHelper purged.

2b496e8

NH-3952: added a measurement for Hazzik reflection method.

2964ad7

hazzik requested changes Feb 28, 2017

View reviewed changes

fredericDelaporte added 2 commits February 28, 2017 16:56

NH-3952: better obsolete message.

a8f6ff3

NH-3952: centralized method cache.

7ae87b2

hazzik requested changes Feb 28, 2017

View reviewed changes

NH-3952: more cleanup.

b746a13

hazzik approved these changes Mar 1, 2017

View reviewed changes

hazzik merged commit 585d83e into nhibernate:master Mar 1, 2017

hazzik modified the milestone: 5.0.0 Mar 1, 2017

fredericDelaporte deleted the NH-3952 branch March 1, 2017 08:43

fredericDelaporte mentioned this pull request Mar 21, 2017

NH-3964 - Refactor reflection patterns #574

Merged

hazzik added the r: Fixed label Aug 3, 2017

fredericDelaporte added the t: Improvement label Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NH-3952: removing usages of NHibernate.Linq.EnumerableHelper #557

NH-3952: removing usages of NHibernate.Linq.EnumerableHelper #557

fredericDelaporte commented Feb 24, 2017 •

edited

Loading

hazzik commented Feb 24, 2017

fredericDelaporte commented Feb 24, 2017 •

edited

Loading

fredericDelaporte commented Feb 27, 2017 •

edited

Loading

hazzik Feb 28, 2017

hazzik Feb 28, 2017

fredericDelaporte Feb 28, 2017

fredericDelaporte commented Feb 28, 2017 •

edited

Loading

hazzik Feb 28, 2017

fredericDelaporte Feb 28, 2017

hazzik Feb 28, 2017

fredericDelaporte Feb 28, 2017

hazzik Feb 28, 2017

fredericDelaporte Feb 28, 2017

fredericDelaporte commented Feb 28, 2017

hazzik commented Mar 1, 2017

hazzik commented Mar 1, 2017

fredericDelaporte commented Mar 1, 2017


		// TODO rename / remove - reflection helper above is better

		[Obsolete("ReflectionHelper is better")]

NH-3952: removing usages of NHibernate.Linq.EnumerableHelper #557

NH-3952: removing usages of NHibernate.Linq.EnumerableHelper #557

Conversation

fredericDelaporte commented Feb 24, 2017 • edited Loading

hazzik commented Feb 24, 2017

fredericDelaporte commented Feb 24, 2017 • edited Loading

fredericDelaporte commented Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Feb 28, 2017

hazzik commented Mar 1, 2017

hazzik commented Mar 1, 2017

fredericDelaporte commented Mar 1, 2017

fredericDelaporte commented Feb 24, 2017 •

edited

Loading

fredericDelaporte commented Feb 24, 2017 •

edited

Loading

fredericDelaporte commented Feb 27, 2017 •

edited

Loading

fredericDelaporte commented Feb 28, 2017 •

edited

Loading