Additional Performance Improvements #359
Conversation
…geProperty] when calling standard multi purpose methods such as ParseArgumentList.
…s are tracked throughout the process to keep our list size minimal.
…msArrayAttr if we are actually an Array type.
…t same lookup multiple times even for simple expressions.
| /// <summary> | ||
| /// Used to provide a lazily accessed Error Message. | ||
| /// </summary> | ||
| public class ErrorMessage |
There was a problem hiding this comment.
This class should be internal, to avoid exposing it as public API.
| /// </summary> | ||
| public class ErrorMessage | ||
| { | ||
| private readonly string _message; |
There was a problem hiding this comment.
_message is unused, and can be removed.
| private readonly ParserArguments _arguments; | ||
| private readonly BindingFlags _bindingCase; | ||
| private readonly MemberFilter _memberFilterCase; | ||
| private readonly Dictionary<MemberTypeKey, MemberInfo[]> _members; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| members = type.FindMembers(memberTypes, flags, _memberFilterCase, memberName); | ||
| if (members.Length == 0) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
| 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; |
There was a problem hiding this comment.
subMembers should be concatenated to the already found members
There was a problem hiding this comment.
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.
| if (members.Length == 0) | ||
| { | ||
| members = Array.Empty<MemberInfo>(); | ||
| } |
There was a problem hiding this comment.
Is this really needed? members is already an empty array of MemberInfo, no?
There was a problem hiding this comment.
I've been in a very memory/GC focused mindset with current full time work so I was wanting to allow the GC to collect the object, but since this is in a very short lived cache I suppose it isn't even necessary.
There was a problem hiding this comment.
It has been removed
| @@ -119,14 +209,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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
|
||
| public int GetHashCode(MemberTypeKey obj) | ||
| { | ||
| return obj._type.GetHashCode() ^ obj._flags.GetHashCode() ^ StringComparer.InvariantCulture.GetHashCode(obj._memberName); |
There was a problem hiding this comment.
StringComparer.InvariantCulture performs a case sensitive comparison, I think you want StringComparer.OrdinalIgnoreCase instead, to match the Equals implementation.
On a side note, it seems that using XOR is not the most reliable way to compute a hash code.
There was a problem hiding this comment.
If you look inside the HashCode class ( which I believe is not available in the .net 4.6.2) its internal implementation is essentially (((x << 7) ^ y) << 7) ^ z. I chose to forego the shifting as the hash codes returned by these GetHashCode calls have a lot of variability and are not heavily centered in the byte range which is why the HashCode class shifts by 7.
There was a problem hiding this comment.
Can you explain the high level idea behind the change?
There was a problem hiding this comment.
In my analysis of how the find applicable method acted I figured out that there were 2 situations that we could leverage to reduce the amount of work done in here.
- Move the final check of best method match to be performed as we find methods. This way instead of doing an n times n number of checks at the end we do approximately n times 2 checks in line where n is the number of possible methods that could fit. Doesn't seem like it would be a big deal, but when we are looking for some of the basic operators I saw the candidate list go up to at least 15 consistently.
- If a method is a perfect match (no casts of any nature needed, all parameters are the exact same type as the arguments being passed) then we don't need to continue to promote methods that need conversion. So instead of fully preparing (which isn't a super cheap operation) a candidate method that needs some argument conversion, we stop the preparation midway to save compute time.
There was a problem hiding this comment.
This class is an excellent idea!
As promised, in #349 I finally have my contribution to the performance wrapped up and ready to go.
The proposed changes break down into addressing the following profiling findings
I also had the goal of improving the FindBestMethods logic to reduce List creation as much as possible. Due to a couple of tests hitting very specific asserts I was unable to get rid of the List completely but the single list that is allocated will only have more than 2 items in extremely rare cases, whereas the old logic regularly could hit 10 or 11 that it then has to reprocess at the end. Additionally, if we have already hit an EXACT match (no type converting, no params usage, etc.) of a method and a candidate method requires some kind of conversion we end the preparing of that method altogether.
Going forward I would like to break the usage of CheckMethodMatchAndPrepareIt logic into the two separate stages
However at this time I didn't have the time to really dig into that logic and figure out exactly when/why things happen.
The only thing I'm not sure on is if we would like the MemberFinder's caching to be something people can turn on/off. I was thinking it is fine to keep on and not even expose that it is happening since the caching only lives per Eval or Parse call and does not persist across multiple calls (unlike the Ncalc library that statically caches its built expressions)
My timings from the same benchmarking logic as show in #349 after improvements is
Timings from before my changes
I'm not sure what's up with the DynamicExpressoParsingOnly N = 20 case since the Eval is proportionally faster.