Improving performance of LanguagePrimitives.TryConvertTo. #7418
Conversation
powercode
force-pushed
the
LanguagePrimTryCompare
branch
2 times, most recently
from
d403c9b to
250a9bf
Compare
Check the conversion rank on the supplied conversion to exit early when we know up front that the conversion will not succeed. Also adding TryCompare, that uses TryConvert in it's implementation.
powercode
force-pushed
the
LanguagePrimTryCompare
branch
from
250a9bf to
6b56c72
Compare
powercode
changed the title
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
It would be nice to see examples that we really get a win.
| if (first is string firstString) | ||
| { | ||
| string secondString = second as string; | ||
| if (secondString == null) |
There was a problem hiding this comment.
Why you skip this? I think we should follow one pattern or roll back the change in previous line 804.
There was a problem hiding this comment.
@iSazonov I think we should be careful about following one pattern too strictly. I think as string is better here because the if condition is secondString == null. Changing it to if (!(second is string secondString)) would make it less readable, which goes against our goal.
There was a problem hiding this comment.
Using different patterns (especially next to each other) also reduces readability - I just stumbled over it.
My suggestion was just to roll back the change at all to old pattern or use one pattern.
| try | ||
| { | ||
| secondConverted = LanguagePrimitives.ConvertTo(second, firstType, culture); | ||
|
|
There was a problem hiding this comment.
Please remove the extra line.
| { | ||
| result = (T)valueToConvert; | ||
| result = variable; | ||
| return true; |
There was a problem hiding this comment.
I'd replace variable with value or valueToReturn.
| result = (T)ConvertTo(valueToConvert, typeof(T), formatProvider); | ||
| if (TryConvertTo(valueToConvert, typeof(T), formatProvider, out var res)) | ||
| { | ||
| result = (T) res; |
There was a problem hiding this comment.
Please remove extra space after (T). Or maybe use as T as more fast?
| } | ||
| result = conversion.Invoke(debase ? PSObject.Base(valueToConvert) : valueToConvert, | ||
| resultType, true, debase ? (PSObject)valueToConvert : null, | ||
| formatProvider, null); |
There was a problem hiding this comment.
Please fix formatting:
result = conversion.Invoke(
debase ? PSObject.Base(valueToConvert) : valueToConvert,
resultType,
recurse: true,
...There was a problem hiding this comment.
Please address the comment and re-format..
| } | ||
|
|
||
| var conversion = FigureConversion(valueToConvert, resultType, out var debase); | ||
| if (conversion.Rank== ConversionRank.None) |
There was a problem hiding this comment.
Please add space before ==.
There was a problem hiding this comment.
Please address the comment and remove extra space
conversion.Rank== ConversionRank.None -> conversion.Rank == ConversionRank.None
| return (T)valueToConvert; | ||
| return variable; | ||
| } | ||
| return (T)ConvertTo(valueToConvert, typeof(T), true, CultureInfo.InvariantCulture, null); |
There was a problem hiding this comment.
Make sense to use return ConvertTo( ... ) as T as more fast?
There was a problem hiding this comment.
I think the perf difference between cast and as is neglectable. And (T)ConvertTo() ensures an exception to be thrown when ConverTo() doesn't really return a T object, which is the expected behavior.
There was a problem hiding this comment.
I think all exceptions occur in ConvertTo(valueToConvert, typeof(T), true, CultureInfo.InvariantCulture, null); and if the method returns a result then the result is always of type T. So formally we can use as T. I see there are performance comparisons of cast and as operators - last is faster - but I don't know whether it is true for .Net Core 2.1.
| public static bool TryConvertTo<T>(object valueToConvert, IFormatProvider formatProvider, out T result) | ||
| { | ||
| result = default(T); | ||
| result = default; |
There was a problem hiding this comment.
I'd keep old code as more readable for the generic method.
| { | ||
| result = default(T); | ||
| result = default; | ||
| try |
There was a problem hiding this comment.
The try/catch can be removed, as the exceptions are taken care of in TryConvertTo(valueToConvert, typeof(T), formatProvider, out var res).
| public static bool TryConvertTo(object valueToConvert, Type resultType, IFormatProvider formatProvider, out object result) | ||
| { | ||
| result = null; | ||
| result = default; |
There was a problem hiding this comment.
I think result = null is more explicit here, as the type of result is object here.
| if (valueToConvert is T variable) | ||
| { | ||
| return (T)valueToConvert; | ||
| return variable; |
There was a problem hiding this comment.
Please use value name as we do below.
| if (TryConvertTo(valueToConvert, typeof(T), formatProvider, out var res)) | ||
| { | ||
| return false; | ||
| result = (T) res; |
There was a problem hiding this comment.
Please remove extra space (T) res -> (T)res.
| } | ||
|
|
||
| var conversion = FigureConversion(valueToConvert, resultType, out var debase); | ||
| if (conversion.Rank== ConversionRank.None) |
There was a problem hiding this comment.
Please address the comment and remove extra space
conversion.Rank== ConversionRank.None -> conversion.Rank == ConversionRank.None
| } | ||
| result = conversion.Invoke(debase ? PSObject.Base(valueToConvert) : valueToConvert, | ||
| resultType, true, debase ? (PSObject)valueToConvert : null, | ||
| formatProvider, null); |
There was a problem hiding this comment.
Please address the comment and re-format..
|
@powercode I made a minor update to a few style issues. I hope you don't mind. They are as follows:
Thank you again for the great contribution! |
|
@powercode Thanks for your contribution! |
3 participants
PR Summary
TryConvertTo is implemented in terms of Convert, but with wrapping exception handling.
This is costly performance wise, so by inlining the implementation of ConvertTo, and getting access to the FigureConversion output. Using that we can determine upfront on at least some occations that a conversion is not available.
Fixes #7420
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests