Refine enum conversion logic in IsEqualStateTrigger

[huoyaoyuan]

Fixes #3681

Created #3681
In current code path, when using IsEqualStateTrigger to compare a string and an enum, it first tries Enum.ToObject which throws, then fallbacks to Convert.ChangeType and success.

When debugger attached with only-my-code off, the exception is caught by the IDE and hurts performance vert much.

PR Type

What kind of change does this PR introduce?

Performance improvement.

What is the current behavior?

Throw, catch, then fallback

What is the new behavior?

Introduce a new code path which doesn't throw if success.

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Other information

I'm using 8.0.0-preview version in WinUI 3 project. How to make this ported into next preview of 8.0.0?

[ghost]

Thanks huoyaoyuan for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[michael-hawker]

@dotMorten have a few minutes to help review this PR with us?

[dotMorten]

ToObject is only allowed if the type is either 'SByte, Int16, Int32, Int64, Byte, UInt16, UInt32, or UInt64.'. See https://docs.microsoft.com/en-us/dotnet/api/system.enum.toobject?view=net-5.0#System_Enum_ToObject_System_Type_System_Object_

So I think it's better if we're really clear on which types we convert, so a throw never would happen. Something along the lines of:

private static object ConvertToEnum(Type enumType, object value)
{
    try
    {
        if (value is string str && Enum.IsDefined(enumType, str))
        {
            return Enum.Parse(enumType, str);
        }
        if((value is SByte || value is Int16 || value is Int32 || value is Int64 || value is Byte || value is UInt16 || value is UInt32 || value is  UInt64) && Enum.IsDefined(enumType, value))
          return  Enum.ToObject(enumType, value);
    }
    catch
    {
    }
    return null;
}

[huoyaoyuan]

I don't think such code bloat looks good. Generally we should focus on common scenarios.
Declaring string in xaml is much easier than others. And also, user shouldn't use two objects that looks no way to be compatible.

If both string parsing and number conversion fails, the conversion will probably fail.

Or, in another way, if the calls are guarded well, the try can be removed.

[huoyaoyuan]

@dotMorten I've refined the parsing logic more:

• First, use object.Equals instead of ==. ReferenceEquals can't handle value types.
  • This technically fixes value types with convertType=false
• For strings, use Enum.TryParse directly to reduce a call. This also handles new cases of flags combination.
• Check typecode before calling Enum.IsDefined and Enum.ToObject. This should prevent throw totally.

[huoyaoyuan]

Well, I think the call to IsDefined can be removed in case of flags combination.

[huoyaoyuan]

The old code seems return false for (Flags1.A | Flags1.B, Flags1.A | Flags1.B).

[Sergio0694]

I agree that these performance improvements would likely not be noticeable given the rest of the overhead (as I've mentioned in my comment too), but that said I thought I'd share more info anyway since Michael asked, and also because I don't see anything wrong with still trying to make the code as best we can in whatever context we're in. As in, those two versions are functionally the same, with just a few characters of difference, but one is faster - so we might as well pick the latter just because 😄

Other than that, I do agree that the main win here is just to tweak the code to avoid relying on exception flow and to just avoid the try/catch block entirely, that'll definitelly be the most noticeable change for consumers, and I think adding more code to prevent that (like the individual type checks @dotMorten illustrated) is the right thing to do 👍

[huoyaoyuan]

because I don't see anything wrong with still trying to make the code as best we can in whatever context we're in. As in, those two versions are functionally the same, with just a few characters of difference, but one is faster

It will get the code much more complex with really little improvement comparing to the scenarios where this pattern get used.

First, the values are already boxed, so there's no way to do boxing elimination like pattern in generic (int)(object)valueOfT.

Then, there must also be a check for enum underlying check, which is not effective even used individual type check. This is literally the biggest reason from me.

[Sergio0694]

"First, the values are already boxed, so there's no way to do boxing elimination like pattern in generic (int)(object)valueOfT."

I think we might be talking about two different things here.
I was only comparing two approaches to check the type of an object (regardless of whether it's a boxed value type or something else). I never proposed a way to avoid the boxing here, as I agree it's not possible in this context 🙂

These are all marginal points anyway, the main thing is to remove the exception, which we all agree on 😄

[huoyaoyuan]

Well, I was considering about behavior compatibility. Previously IsDefined will throw for a number with type other than defined type. But anyway, we are refining the behavior to be better now. It reasonable to accept int for byte based enum. I'm going to adjust for this.

[Sergio0694]

But anyway, we are refining the behavior to be better now. It reasonable to accept int for byte based enum.

Definitely agree with that, especially in a UI-focused API.
From my experience not many devs know that enums can even have a different backing type in the first place, and in general that's a feature I'd expect to be used more by lower-level devs, rather than frontend devs. Besides, if the converter exposes the target value as object and you pass an integer from XAML, it'll be boxed as an int anyway, so I agree that giving the API some more internal wiggle room to still work fine in these cases would be nice to have 👍

[Kyaa-dost]

@huoyaoyuan Thanks for submitting the PR ❤️

Can you please create an issue for this PR as well so we can keep it in our queue for tracking purposes?

[huoyaoyuan]

Sure. I can also imagine some more improvements.

[Sergio0694]

Just a couple nits 😄

[ghost]

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

[Kyaa-dost]

@huoyaoyuan just checking to see if everything is going well with regards to this PR? or do you have any questions regarding the feedback provided above?

[huoyaoyuan]

@Kyaa-dost I've updated with the suggestion, but not exact same syntax. You can review the latest commit.

Adding unit tests seems out of scope of this PR. I'm fine now.

[michael-hawker]

@Sergio0694 is this all good now for you?

[Sergio0694]

@michael-hawker Yup! The main issue was mostly just relying on exceptions for the control flow which was slowing down everything anyway, and this PR fixed that issue just fine, so seems good to me now! 🙂

[michael-hawker]

@Sergio0694 @dotMorten if you want to sign-off on this, then we can merge it. Thanks!

[michael-hawker]

@RosarioPulella want to take a look at this one too? I think @Sergio0694 was onboard, but waiting on @dotMorten, but we haven't heard back and we should get this merged for 7.0 today.

[Sergio0694]

Minor note: now that we no longer have Enum.IsDefined, this PR introduces a small behavior change. As in, if value happens to be a boxed number that is outside of the defined range for the current enum type, this will now still succeed and just return an undefined (boxed) enum value, whereas before this PR it'd return null instead.

I mean, this should likely not happen anyway, but just worth pointing it out.
I'm fine with just leaving up to consumers to ensure input raw numeric values are in fact in range.

[michael-hawker]

I think that's fine. As a StateTrigger this should be somethings Devs are working with in a VSM, so it'd be a bit odd to try and use something that wasn't defined?

[Sergio0694]

Yup agreed, just figured I'd point that out to clarify since it's effectively a (small) functional change though 🙂

[dotMorten]

Sorry for the slow response on my side here. It slipped through the cracks of notification overload ;-) I think the changes should be fine.