-
Notifications
You must be signed in to change notification settings - Fork 933
Avoid lambda compilation as much as possible #2957
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
bahusoid
merged 16 commits into
nhibernate:master
from
gokhanabatay:AvoidLambdaCompilation
Dec 14, 2021
Merged
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
bb1bf45
Merge pull request #33 from nhibernate/master
gokhanabatay f1586a9
Avoid lambda compilation as much as possible, concurrent Compile() ca…
043df3c
convert expression avoid as much as cast if its possible
cbb0b5c
convert method call parameter fix
5c8ad16
Add runtime shim for LambdaExpression.Compile(bool)
hazzik 9c6808c
Update src/NHibernate/Impl/ExpressionProcessor.cs
gokhanabatay 32d674d
Some refactoring. No functional changes
bahusoid ea946ea
Revert "Add runtime shim for LambdaExpression.Compile(bool)" - it's n…
bahusoid af8212e
Fixed compiling of int? -> int due my last refactoring
bahusoid 4ee6a7f
whitespaces
bahusoid fcb1ecb
Refactor getObj to be more generic
hazzik caf970c
Add tests for int to long? and decimal?
hazzik 4d29727
Add tests to upcast nullable int? -> long?
hazzik 9c7a726
test case expression double compile issue fixed
0a83347
Whitespaces
hazzik 5cc5214
Add more tests
hazzik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
292 changes: 292 additions & 0 deletions
292
src/NHibernate.Test/Criteria/Lambda/ExpressionProcessorFindValueFixture.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,292 @@ | ||
using System; | ||
using System.Linq.Expressions; | ||
using System.Reflection; | ||
using NHibernate.Impl; | ||
using NUnit.Framework; | ||
|
||
namespace NHibernate.Test.Criteria.Lambda | ||
{ | ||
public static class DateTimeExtensions | ||
{ | ||
public static long ToLongDateTime(this DateTime date) | ||
{ | ||
return Convert.ToInt64(date.ToString("yyyyMMddHHmmss")); | ||
} | ||
} | ||
|
||
[TestFixture] | ||
public class ExpressionProcessorFindValueFixture | ||
{ | ||
private static object GetValue<T>(Expression<Func<T>> expression) | ||
{ | ||
try | ||
{ | ||
return ExpressionProcessor.FindValue(expression.Body); | ||
} | ||
catch (TargetInvocationException e) | ||
{ | ||
throw e.InnerException; | ||
} | ||
} | ||
|
||
private static int GetIntegerDate() | ||
{ | ||
return Convert.ToInt32(DateTime.Now.ToString("yyyyMMdd")); | ||
} | ||
|
||
[Test] | ||
public void StaticPropertyInstanceMethodCall() | ||
{ | ||
var actual = GetValue(() => DateTime.Now.ToString("yyyyMMdd")); | ||
var expected = DateTime.Now.ToString("yyyyMMdd"); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void StaticPropertyInstanceMultipleMethodCall() | ||
{ | ||
var actual = GetValue(() => DateTime.Now.AddDays(365).ToString("yyyyMMdd")); | ||
var expected = DateTime.Now.AddDays(365).ToString("yyyyMMdd"); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void StaticPropertyInstanceMethodCallThenCast() | ||
{ | ||
var actual = GetValue(() => Convert.ToInt32(DateTime.Now.AddDays(365).ToString("yyyyMMdd"))); | ||
var expected = Convert.ToInt32(DateTime.Now.AddDays(365).ToString("yyyyMMdd")); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void StaticMethodCall() | ||
{ | ||
var actual = GetValue(() => GetIntegerDate()); | ||
var expected = GetIntegerDate(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void ExtensionMethodCall() | ||
{ | ||
var date = DateTime.Now; | ||
var actual = GetValue(() => date.ToLongDateTime()); | ||
var expected = date.ToLongDateTime(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void NestedPropertyAccess() | ||
{ | ||
var animal = new { Snake = new { Animal = new { Name = "Scorpion" } } }; | ||
var actual = GetValue(() => animal.Snake.Animal.Name); | ||
var expected = animal.Snake.Animal.Name; | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void GuidToStringCast() | ||
{ | ||
var guid = Guid.NewGuid(); | ||
Expression<Func<string>> expression = () => $"{guid}"; | ||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void NestedPropertyToIntegerCast() | ||
{ | ||
var animal = new { Snake = new { Animal = new { Weight = 9.89 } } }; | ||
Expression<Func<int>> expression = () => (int) animal.Snake.Animal.Weight; | ||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void NestedPropertyToIntegerConvert() | ||
{ | ||
var animal = new { Snake = new { Animal = new { Weight = 9.89 } } }; | ||
Expression<Func<int>> expression = () => Convert.ToInt32(animal.Snake.Animal.Weight); | ||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void NullToIntegerCastFails() | ||
{ | ||
object value = null; | ||
Expression<Func<int>> expression = () => (int) value; | ||
|
||
Assert.Throws<NullReferenceException>(() => GetValue(expression)); | ||
|
||
//Check with expression compile and invoke | ||
Assert.Throws<NullReferenceException>(() => expression.Compile().Invoke()); | ||
} | ||
|
||
[Test] | ||
public void NullableIntegerToIntegerCastFails() | ||
{ | ||
int? value = null; | ||
Expression<Func<int>> expression = () => (int) value; | ||
|
||
Assert.Throws<InvalidOperationException>(() => GetValue(expression)); | ||
|
||
//Check with expression compile and invoke | ||
Assert.Throws<InvalidOperationException>(() => expression.Compile().Invoke()); | ||
} | ||
|
||
[Test] | ||
public void NullableIntegerToIntegerCast() | ||
{ | ||
int? value = 1; | ||
Expression<Func<int>> expression = () => (int) value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void NullableIntegerToNullableLongImplicitCast() | ||
{ | ||
int? value = 1; | ||
Expression<Func<long?>> expression = () => value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void IntegerToIntegerCast() | ||
{ | ||
int value = 1; | ||
Expression<Func<int>> expression = () => (int) value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void IntegerToNullableIntegerImplicitCast() | ||
{ | ||
int value = 12345; | ||
Expression<Func<int?>> expression = () => value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void IntegerToNullableLongImplicitCast() | ||
{ | ||
int value = 12345; | ||
Expression<Func<long?>> expression = () => value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void IntegerToNullableDecimalImplicitCast() | ||
{ | ||
int value = 12345; | ||
Expression<Func<decimal?>> expression = () => value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void Int16ToIntegerImplicitCast() | ||
{ | ||
short value = 12345; | ||
Expression<Func<int>> expression = () => value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void NullableDecimalToDecimalCast() | ||
{ | ||
decimal? value = 9.89m; | ||
Expression<Func<decimal>> expression = () => (decimal) value; | ||
|
||
var actual = GetValue(expression); | ||
|
||
//Check with expression compile and invoke | ||
var expected = expression.Compile().Invoke(); | ||
|
||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
[Test] | ||
public void StringToIntegerCastFails() | ||
{ | ||
object value = "Abatay"; | ||
Expression<Func<int>> expression = () => (int) value; | ||
|
||
Assert.Throws<InvalidCastException>(() => GetValue(expression)); | ||
|
||
//Check with expression compile and invoke | ||
Assert.Throws<InvalidCastException>(() => expression.Compile().Invoke()); | ||
} | ||
|
||
[Test] | ||
public void BooleanToCharCastFails() | ||
{ | ||
object isTrue = true; | ||
Expression<Func<char>> expression = () => (char) isTrue; | ||
|
||
Assert.Throws<InvalidCastException>(() => GetValue(expression)); | ||
|
||
//Check with expression compile and invoke | ||
Assert.Throws<InvalidCastException>(() => expression.Compile().Invoke()); | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
using System; | ||
using System.Linq.Expressions; | ||
|
||
namespace NHibernate.Impl | ||
{ | ||
#if NET461 | ||
internal static class LambdaExpressionExtensions | ||
{ | ||
public static Delegate Compile(this LambdaExpression expression, bool preferInterpretation) => | ||
expression.Compile(); //Concurrent Compile() call causes "Garbage Collector" suspend all threads too often | ||
} | ||
#endif | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would fall into 'lambda compile' path. Do we want to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to fix it bu convert is to much tricky dou you have any idea to fix it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said in #2957 (comment)
Let's start from something simple but safe and make it more complex and sophisticated in other PRs. Unless you already have in mind some simple fix that you want to stick in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a variation of
ConvertType
in the expression processor. Convert for primitive types (IsPrimitive
):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do it in another PR. It doesn't look as straightforward fix to me. For instance Boolean type and value overflows might behave differently in runtime vs ChangeType.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @bahusoid, in my test I was stuck with
NestedPropertyToIntegerCast
andStringToIntegerCastFails
failed withConvert.ChangeType
Such as
Convert.ChangeType(9.89m, typeof(int)) = 10, (int)9.89m = 9
Convert.ChangeType(true, typeof(int)) = 1, (int)true = fails
I also tried with
T Cast<T>(object value) => return (T)value;
store static instance of CastMethodInfo and make generic method before invoke but this was also failed. Which may run for primitive cases, for performance i am not sure of it? @hazzik