Skip to content

Additional Performance Improvements #359

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Additional Performance Improvements #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3eb8e42
Using a lazy access pattern instead of accessing ErrorMessages.[Messa…
holdenmai Apr 13, 2025
42fbaec
Updating the FindBestMethod logic to reduce how many candidate method…
holdenmai Apr 13, 2025
59c6cb9
Updating HasParamsArrayType to only check for the existence of a Para…
holdenmai Apr 13, 2025
4576ce6
Adding some caching logic to our member lookup as we can hit the exac…
holdenmai Apr 13, 2025
cb7ea5f
Update ErrorMessage from PR comments
holdenmai May 17, 2025
1b000f3
Update MemberFinder with PR comments.
holdenmai May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 29 additions & 29 deletions src/DynamicExpresso.Core/Parsing/Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ private Expression Parse()
{
Expression expr = ParseExpressionSegment(_arguments.ExpressionReturnType);

ValidateToken(TokenId.End, ErrorMessages.SyntaxError);
ValidateToken(TokenId.End, ErrorMessage.SyntaxError);
return expr;
}

Expand Down Expand Up @@ -170,7 +170,7 @@ private ParameterWithPosition[] ParseLambdaParameterList()
var parameters = _token.id != TokenId.CloseParen ? ParseLambdaParameters() : new ParameterWithPosition[0];
if (hasOpenParen)
{
ValidateToken(TokenId.CloseParen, ErrorMessages.CloseParenOrCommaExpected);
ValidateToken(TokenId.CloseParen, ErrorMessage.CloseParenOrCommaExpected);
NextToken();
}

Expand Down Expand Up @@ -255,7 +255,7 @@ private Expression ParseConditional()
{
NextToken();
var expr1 = ParseExpressionSegment();
ValidateToken(TokenId.Colon, ErrorMessages.ColonExpected);
ValidateToken(TokenId.Colon, ErrorMessage.ColonExpected);
NextToken();
var expr2 = ParseExpressionSegment();
expr = GenerateConditional(expr, expr1, expr2, errorPos);
Expand Down Expand Up @@ -991,10 +991,10 @@ private static Expression CreateLiteral(object value)

private Expression ParseParenExpression()
{
ValidateToken(TokenId.OpenParen, ErrorMessages.OpenParenExpected);
ValidateToken(TokenId.OpenParen, ErrorMessage.OpenParenExpected);
NextToken();
Expression innerParenthesesExpression = ParseExpressionSegment();
ValidateToken(TokenId.CloseParen, ErrorMessages.CloseParenOrOperatorExpected);
ValidateToken(TokenId.CloseParen, ErrorMessage.CloseParenOrOperatorExpected);

var constExp = innerParenthesesExpression as ConstantExpression;
if (constExp != null && constExp.Value is Type)
Expand Down Expand Up @@ -1108,11 +1108,11 @@ private Expression ParseDefaultOperator()
{
NextToken();

ValidateToken(TokenId.OpenParen, ErrorMessages.OpenParenExpected);
ValidateToken(TokenId.OpenParen, ErrorMessage.OpenParenExpected);
NextToken();
ValidateToken(TokenId.Identifier);
var type = ParseKnownType();
ValidateToken(TokenId.CloseParen, ErrorMessages.CloseParenOrCommaExpected);
ValidateToken(TokenId.CloseParen, ErrorMessage.CloseParenOrCommaExpected);
NextToken();

return Expression.Default(type);
Expand Down Expand Up @@ -1162,7 +1162,7 @@ private Expression GenerateConditionalDynamic(Expression test, Expression expr1,
private Expression ParseNew()
{
NextToken();
ValidateToken(TokenId.Identifier, ErrorMessages.IdentifierExpected);
ValidateToken(TokenId.Identifier, ErrorMessage.IdentifierExpected);

var newType = ParseKnownType();
var args = new Expression[0];
Expand All @@ -1181,7 +1181,7 @@ private Expression ParseNew()
else
{
// no aguments: expect an object initializer
ValidateToken(TokenId.OpenCurlyBracket, ErrorMessages.OpenCurlyBracketExpected);
ValidateToken(TokenId.OpenCurlyBracket, ErrorMessage.OpenCurlyBracketExpected);
}

var applicableConstructors = MethodResolution.FindBestMethod(newType.GetConstructors(), args);
Expand All @@ -1202,17 +1202,17 @@ private Expression ParseNew()

private Expression[] ParseArrayInitializerList()
{
return ParseArgumentList(TokenId.OpenCurlyBracket, ErrorMessages.OpenCurlyBracketExpected,
TokenId.CloseCurlyBracket, ErrorMessages.CloseCurlyBracketExpected,
return ParseArgumentList(TokenId.OpenCurlyBracket, ErrorMessage.OpenCurlyBracketExpected,
TokenId.CloseCurlyBracket, ErrorMessage.CloseCurlyBracketExpected,
allowTrailingComma: true);
}

private Expression ParseWithObjectInitializer(NewExpression newExpr, Type newType)
{
ValidateToken(TokenId.OpenCurlyBracket, ErrorMessages.OpenCurlyBracketExpected);
ValidateToken(TokenId.OpenCurlyBracket, ErrorMessage.OpenCurlyBracketExpected);
NextToken();
var initializedInstance = ParseMemberAndInitializerList(newExpr, newType);
ValidateToken(TokenId.CloseCurlyBracket, ErrorMessages.CloseCurlyBracketExpected);
ValidateToken(TokenId.CloseCurlyBracket, ErrorMessage.CloseCurlyBracketExpected);
NextToken();
return initializedInstance;
}
Expand Down Expand Up @@ -1249,7 +1249,7 @@ private Expression ParseMemberAndInitializerList(NewExpression newExpr, Type new

private void ParsePossibleMemberBinding(Type newType, int originalPos, List<MemberBinding> bindingList, List<Expression> actions, ParameterExpression instance, bool allowCollectionInit)
{
ValidateToken(TokenId.Identifier, ErrorMessages.IdentifierExpected);
ValidateToken(TokenId.Identifier, ErrorMessage.IdentifierExpected);

var propertyOrFieldName = _token.text;
var member = _memberFinder.FindPropertyOrField(newType, propertyOrFieldName, false);
Expand Down Expand Up @@ -1282,7 +1282,7 @@ private void ParsePossibleMemberBinding(Type newType, int originalPos, List<Memb
}
NextToken();

ValidateToken(TokenId.Equal, ErrorMessages.EqualExpected);
ValidateToken(TokenId.Equal, ErrorMessage.EqualExpected);
NextToken();

var value = ParseExpressionSegment();
Expand Down Expand Up @@ -1323,7 +1323,7 @@ private void ParseCollectionInitalizer(Type newType, int originalPos, List<Membe
SetTextPos(pos);
ParseExpressionSegment();
}
actions.Add(ParseMethodInvocation(newType, instance, _token.pos, "Add", TokenId.OpenCurlyBracket, ErrorMessages.OpenCurlyBracketExpected, TokenId.CloseCurlyBracket, ErrorMessages.CloseCurlyBracketExpected));
actions.Add(ParseMethodInvocation(newType, instance, _token.pos, "Add", TokenId.OpenCurlyBracket, ErrorMessage.OpenCurlyBracketExpected, TokenId.CloseCurlyBracket, ErrorMessage.CloseCurlyBracketExpected));
}
else
{
Expand Down Expand Up @@ -1499,7 +1499,7 @@ private Type ParseArrayRankSpecifier(Type type)
NextToken();
}

ValidateToken(TokenId.CloseBracket, ErrorMessages.CloseBracketOrCommaExpected);
ValidateToken(TokenId.CloseBracket, ErrorMessage.CloseBracketOrCommaExpected);
ranks.Push(rank);
NextToken();
}
Expand Down Expand Up @@ -1527,7 +1527,7 @@ private List<Type> ParseTypeArgumentList()
args = new List<Type>(new Type[arity]);
}

ValidateToken(TokenId.GreaterThan, ErrorMessages.CloseTypeArgumentListExpected);
ValidateToken(TokenId.GreaterThan, ErrorMessage.CloseTypeArgumentListExpected);
return args;
}

Expand Down Expand Up @@ -1569,7 +1569,7 @@ private Expression ParseTypeKeyword(Type type)
return Expression.Constant(type);
}

ValidateToken(TokenId.Dot, ErrorMessages.DotOrOpenParenExpected);
ValidateToken(TokenId.Dot, ErrorMessage.DotOrOpenParenExpected);
NextToken();
return ParseMemberAccess(type, null);
}
Expand Down Expand Up @@ -1684,11 +1684,11 @@ private Expression GeneratePropertyOrFieldExpression(Type type, Expression insta

private Expression ParseMethodInvocation(Type type, Expression instance, int errorPos, string methodName)
{
return ParseMethodInvocation(type, instance, errorPos, methodName, TokenId.OpenParen, ErrorMessages.OpenParenExpected, TokenId.CloseParen, ErrorMessages.CloseParenOrCommaExpected);
return ParseMethodInvocation(type, instance, errorPos, methodName, TokenId.OpenParen, ErrorMessage.OpenParenExpected, TokenId.CloseParen, ErrorMessage.CloseParenOrCommaExpected);

}

private Expression ParseMethodInvocation(Type type, Expression instance, int errorPos, string methodName, TokenId open, string openExpected, TokenId close, string closeExpected)
private Expression ParseMethodInvocation(Type type, Expression instance, int errorPos, string methodName, TokenId open, ErrorMessage openExpected, TokenId close, ErrorMessage closeExpected)
{
var args = ParseArgumentList(open, openExpected, close, closeExpected);

Expand Down Expand Up @@ -1818,8 +1818,8 @@ private static Expression ParseDynamicIndex(Type type, Expression instance, Expr
return Expression.Dynamic(new LateGetIndexCallSiteBinder(), typeof(object), argsDynamic);
}

private Expression[] ParseArgumentList(TokenId openToken, string missingOpenTokenMsg,
TokenId closeToken, string missingCloseTokenMsg,
private Expression[] ParseArgumentList(TokenId openToken, ErrorMessage missingOpenTokenMsg,
TokenId closeToken, ErrorMessage missingCloseTokenMsg,
bool allowTrailingComma = false)
{
ValidateToken(openToken, missingOpenTokenMsg);
Expand All @@ -1840,15 +1840,15 @@ private Expression[] ParseArgumentList(TokenId openToken, string missingOpenToke

private Expression[] ParseArgumentList()
{
return ParseArgumentList(TokenId.OpenParen, ErrorMessages.OpenParenExpected,
TokenId.CloseParen, ErrorMessages.CloseParenOrCommaExpected);
return ParseArgumentList(TokenId.OpenParen, ErrorMessage.OpenParenExpected,
TokenId.CloseParen, ErrorMessage.CloseParenOrCommaExpected);
}

private Expression ParseElementAccess(Expression expr)
{
var errorPos = _token.pos;
var args = ParseArgumentList(TokenId.OpenBracket, ErrorMessages.OpenParenExpected,
TokenId.CloseBracket, ErrorMessages.CloseBracketOrCommaExpected);
var args = ParseArgumentList(TokenId.OpenBracket, ErrorMessage.OpenParenExpected,
TokenId.CloseBracket, ErrorMessage.CloseBracketOrCommaExpected);
if (expr.Type.IsArray)
{
if (expr.Type.GetArrayRank() != args.Length)
Expand Down Expand Up @@ -2571,7 +2571,7 @@ private void NextToken()

private string GetIdentifier()
{
ValidateToken(TokenId.Identifier, ErrorMessages.IdentifierExpected);
ValidateToken(TokenId.Identifier, ErrorMessage.IdentifierExpected);
var id = _token.text;
if (id.Length > 1 && id[0] == '@')
id = id.Substring(1);
Expand All @@ -2585,7 +2585,7 @@ private void ValidateDigit()
}

// ReSharper disable once UnusedParameter.Local
private void ValidateToken(TokenId t, string errorMessage)
private void ValidateToken(TokenId t, ErrorMessage errorMessage)
{
if (_token.id != t)
throw ParseException.Create(_token.pos, errorMessage);
Expand Down
118 changes: 102 additions & 16 deletions src/DynamicExpresso.Core/Reflection/MemberFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,38 +9,119 @@ namespace DynamicExpresso.Reflection
{
internal class MemberFinder
{
private struct MemberTypeKey
{
public Type _type;
public string _memberName;
public BindingFlags _flags;

public MemberTypeKey(Type type, string memberName, BindingFlags flags)
{
_type = type;
_memberName = memberName;
_flags = flags;
}
}

private class MemberTypeKeyEqualityComparer : IEqualityComparer<MemberTypeKey>
{
public static readonly MemberTypeKeyEqualityComparer Instance = new MemberTypeKeyEqualityComparer();

public bool Equals(MemberTypeKey x, MemberTypeKey y)
{
return x._type.Equals(y._type) && x._flags == y._flags && x._memberName.Equals(y._memberName, StringComparison.Ordinal);
}


public int GetHashCode(MemberTypeKey obj)
{
return obj._type.GetHashCode() ^ obj._flags.GetHashCode() ^ obj._memberName.GetHashCode();
}
}

private class MemberTypeKeyCaseInsensitiveEqualityComparer : IEqualityComparer<MemberTypeKey>
{
public static readonly MemberTypeKeyCaseInsensitiveEqualityComparer Instance = new MemberTypeKeyCaseInsensitiveEqualityComparer();
public bool Equals(MemberTypeKey x, MemberTypeKey y)
{
return x._type.Equals(y._type) && x._flags == y._flags && x._memberName.Equals(y._memberName, StringComparison.OrdinalIgnoreCase);
}

public int GetHashCode(MemberTypeKey obj)
{
return obj._type.GetHashCode() ^ obj._flags.GetHashCode() ^ StringComparer.OrdinalIgnoreCase.GetHashCode(obj._memberName);
}
}

private readonly ParserArguments _arguments;
private readonly BindingFlags _bindingCase;
private readonly MemberFilter _memberFilterCase;
private readonly Dictionary<MemberTypeKey, MemberInfo[]> _members;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's worth caching the members. It's only useful if the same member is accessed multiple times in the same expression, but I'm not sure that's frequent in regular use of the Library.

I've still reviewed the proposal.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, agree. Accessing the same member I suppose it is not so common.
Maybe we can think of a cache per Interpreter instance. But we should must be careful to design it, because Interpreter class can be used by multiple threads. But I suggest to put this optimization in a separate PR, to eval it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I included this caching logic was because I found during analysis that we would frequently hit the same lookup if we were doing any non trivial expressions, particularly ones that had more than one of the same call happening, for example x + y + z, it would perform the lookup each time. If I recall correctly I think we were also hitting the same lookup more than once even on more trivial expressions like x + y but I need to verify that again.

However the major thing I found is that with how the actual lookup happens in type.FindMembers if we ever hit any lookup more than 1 time we are instantly better off for having the catching.


public MemberFinder(ParserArguments arguments)
{
_arguments = arguments;
_bindingCase = arguments.Settings.CaseInsensitive ? BindingFlags.IgnoreCase : BindingFlags.Default;
_memberFilterCase = arguments.Settings.CaseInsensitive ? Type.FilterNameIgnoreCase : Type.FilterName;
_members = new Dictionary<MemberTypeKey, MemberInfo[]>(arguments.Settings.CaseInsensitive ? (IEqualityComparer<MemberTypeKey>)MemberTypeKeyCaseInsensitiveEqualityComparer.Instance : MemberTypeKeyEqualityComparer.Instance);
}

public MemberInfo FindPropertyOrField(Type type, string memberName, bool staticAccess)
{
var flags = BindingFlags.Public | BindingFlags.DeclaredOnly |
(staticAccess ? BindingFlags.Static : BindingFlags.Instance) | _bindingCase;

foreach (var t in SelfAndBaseTypes(type))
//Not looping here because FindMembers loops internally and returns us the highest level member
var members = FindMembers(type, MemberTypes.Property | MemberTypes.Field, flags, memberName);
if (members.Length != 0)
{
var members = t.FindMembers(MemberTypes.Property | MemberTypes.Field, flags, _memberFilterCase, memberName);
if (members.Length != 0)
return members[0];
return members[0];
}
return null;
}

private MemberInfo[] FindMembers(Type type, MemberTypes memberTypes, BindingFlags flags, string memberName)
{
var key = new MemberTypeKey(type, memberName, flags);
if (_members.TryGetValue(key, out var members))
{
if (members != null)
{
return members;
}
}
members = type.FindMembers(memberTypes, flags, _memberFilterCase, memberName);
if (members.Length == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

For methods, you should always consider the base types. This would also solve the comment in FindMethods, since FindMembers would find all methods of the same name in the type's hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was implemented in this way to preserve the same results as the previous implementation.

Primarily we wouldn't want to consider the base type at this location because we would end up getting ambiguous method matches in the case of "new" and "override"

{
foreach (var v in SelfAndBaseTypes(type))
{
if (v == type)
{
continue;
}
var subMembers = FindMembers(v, memberTypes, flags, memberName);
if (members.Length == 0 && subMembers.Length > 0)
{
//We don't break here because there is a possibility that somebody outside here is also doing an additional tree prioritization (See FindMethods)
members = subMembers;
Copy link
Contributor

Choose a reason for hiding this comment

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

subMembers should be concatenated to the already found members

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See other comment. If concatenated with different levels of hierarchy we could get ambiguous fields, properties, etc .

The reason for the assign from submemebrs is that it gives the closest match. For example if somebody had 6 levels of inheritance (yes, code smell but bear with me) and we were looking for a member named Foo that was only on the base class this should allow us to jump down the inheritance without having to loop through the whole inheritance cycle.

However I think my comment/lack of break is incorrect in the code. I will make a few more tests to verify that members on base types are correctly found.

}
}
}
_members[key] = members;
return members;
}

public IList<MethodData> FindMethods(Type type, string methodName, bool staticAccess, Expression[] args)
{
var flags = BindingFlags.Public | BindingFlags.DeclaredOnly |
(staticAccess ? BindingFlags.Static : BindingFlags.Instance) | _bindingCase;
//Yes, FindMembers loops internally, but there are certain circumstances where we end up also needing this
//An example is Type.GetMethods(), where there are multiple overloads on the base Type class
//and one override on a derived class. The DeclaredOnly flag will return the single method on the derived class unless we go lower.
//The existing unit tests do pass if we get rid of DeclaredOnly and remove the loop, but I'm not sure of the actual impact that may have.
foreach (var t in SelfAndBaseTypes(type))
{
var members = t.FindMembers(MemberTypes.Method, flags, _memberFilterCase, methodName);
var members = FindMembers(t, MemberTypes.Method, flags, methodName);
var applicableMethods = MethodResolution.FindBestMethod(members.Cast<MethodBase>(), args);

if (applicableMethods.Count > 0)
Expand Down Expand Up @@ -100,14 +181,19 @@ private static IEnumerable<Type> SelfAndBaseTypes(Type type)
{
if (type.IsInterface)
{
var types = new List<Type>();
AddInterface(types, type);

types.Add(typeof(object));
var types = new HashSet<Type>();
foreach (var v in AddInterface(types, type))
{
yield return v;
}

return types;
yield return typeof(object);
yield break;
}
foreach (var t in SelfAndBaseClasses(type))
{
yield return t;
}
return SelfAndBaseClasses(type);
}

private static IEnumerable<Type> SelfAndBaseClasses(Type type)
Expand All @@ -119,14 +205,14 @@ private static IEnumerable<Type> SelfAndBaseClasses(Type type)
}
}

private static void AddInterface(List<Type> types, Type type)
private static IEnumerable<Type> AddInterface(HashSet<Type> types, Type type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, with the current implementation, types is always empty since you never call types.Add(). Can you just fill types instead of returning an IEnumerable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch.

The reason for not filling the HashSet and returning that set is that we are not 100% guaranteed that the order in which we populate the HashSet is the order in which it was added, so if it ever varied across executions we could find ourselves in an extremely difficult to replicate scenario.

{
if (!types.Contains(type))
if (types.Add(type))
{
types.Add(type);
foreach (var t in type.GetInterfaces())
yield return type;
foreach (var i in type.GetInterfaces().SelectMany(x => AddInterface(types, x)))
{
AddInterface(types, t);
yield return i;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/DynamicExpresso.Core/Reflection/ReflectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public static Type GetActionType(int parameterCount)

public static bool HasParamsArrayType(ParameterInfo parameterInfo)
{
return parameterInfo.IsDefined(typeof(ParamArrayAttribute), false);
return parameterInfo.ParameterType.IsArray && parameterInfo.IsDefined(typeof(ParamArrayAttribute), false);
}

public static Type GetParameterType(ParameterInfo parameterInfo)
Expand Down
Loading