You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The present implementation of the union converter is relatively efficient once it is initialized, but initialization is very expensive and induces multiple arguably unnecessary penalties, both in time and memory usage. This issue serves as a list of possible improvements, including ideas on how to solve them; but none of these are compulsory to be implemented and it may well turn out that a different approach is better suited to the problem.
1. Multiple generic instantiations
As noticed by @OoLunar , OneOf<string, long> and OneOf<long, string> are two distinct generic instantiations that will cause the static constructor to execute twice. While this is not a significant issue in the core library by itself, it may become an issue in user-defined models, or if the core library expands to a point where it includes many different OneOf properties - and to a point where changing the order of generic parameter becomes an optimization.
2. Excessive re-ordering of small unions
The current implementation does not distinguish between the arity of the given union, and as such performs the expensive ordering and sorting even on single-member unions and potentially excessive ordering on sorting on double-member unions. One trivial-ish change would be to write an implementation for a single-member union that doesn't add any logic unto itself, but dealing with this for double-member unions may be difficult and there will likely always be space for improvement.
3. Construction by MethodInfo
This change would actually benefit runtime speed - presently, the converter uses MethodInfo dynamic invokations to call the FromTN methods on OneOf, using literally anything more specific may well be beneficial. This may be accomplished through System.Linq.Expressions, but needs further experimentation. Another useful side-effect of removing MethodInfo would be removing an array allocation for the second parameter of MethodInfo.Invoke.
Nothing of this strictly needs to happen, but it would likely be beneficial to the library itself, and definitely to users seeking to extend the library models, if it were to happen.
The text was updated successfully, but these errors were encountered:
Summary
The present implementation of the union converter is relatively efficient once it is initialized, but initialization is very expensive and induces multiple arguably unnecessary penalties, both in time and memory usage. This issue serves as a list of possible improvements, including ideas on how to solve them; but none of these are compulsory to be implemented and it may well turn out that a different approach is better suited to the problem.
1. Multiple generic instantiations
As noticed by @OoLunar ,
OneOf<string, long>
andOneOf<long, string>
are two distinct generic instantiations that will cause the static constructor to execute twice. While this is not a significant issue in the core library by itself, it may become an issue in user-defined models, or if the core library expands to a point where it includes many different OneOf properties - and to a point where changing the order of generic parameter becomes an optimization.2. Excessive re-ordering of small unions
The current implementation does not distinguish between the arity of the given union, and as such performs the expensive ordering and sorting even on single-member unions and potentially excessive ordering on sorting on double-member unions. One trivial-ish change would be to write an implementation for a single-member union that doesn't add any logic unto itself, but dealing with this for double-member unions may be difficult and there will likely always be space for improvement.
3. Construction by MethodInfo
This change would actually benefit runtime speed - presently, the converter uses MethodInfo dynamic invokations to call the FromTN methods on OneOf, using literally anything more specific may well be beneficial. This may be accomplished through
System.Linq.Expressions
, but needs further experimentation. Another useful side-effect of removing MethodInfo would be removing an array allocation for the second parameter ofMethodInfo.Invoke
.Nothing of this strictly needs to happen, but it would likely be beneficial to the library itself, and definitely to users seeking to extend the library models, if it were to happen.
The text was updated successfully, but these errors were encountered: