Use Math to get min scale and max scale in Viewbox.ComputeScaleFactor

[h82258652]

No description provided.

[miloush]

What is the performance impact of this change?

[h82258652]

@miloush Due to the method call, the performance is a little worse than before, but the code is clearer. I don't know should I pr this. If we need to consider the performance, there are lots of Math calls that can be replaced.

[fabiant3]

It's unfortunate but we don't have automated perf tests running on WPF NET5. This is critical path code, so I wouldn't spend time on this PR at the moment.

[miloush]

@h82258652 That's not even true in general, the method call might get optimized away. It depends on the compiler, jitter, platform, where the values come from, their data types etc.

Being able to state either "under conditions X the performance improvement is Y and every WPF app hits this line 100 times at start-up" or "we get more readable/consistent code for the performance/size cost of X and the code is rarely ever hit" helps people to make an informed decision on whether the change is worth the effort and risk.

[gfoidl]

Due to the method call, the performance is a little worse than before

FYI with dotnet/runtime#33851 Math.Min and Math.Max will be inlined.
It still may be a bit slower due the handling of NaNs and other edge-cases, which the simple comparison doesn't have. Depending on the CPU the branch-predictor may do a good job, so this handling of the edge-cases may not show up in perf-numbers.
I'd go with the cleaner code as this PR suggests.

[rchauhan18]

I performed a perf test and it shows that using Math.Max/Math.Min is 4-5 times slower then the default. Here are the results:

If we take this PR now, the only output we get is a clean code at the cost of reduced performance. As such, we are closing this PR until the time there is a change in underlying functions which has atleast same or comparable performance.

[miloush]

Not mentioning the fact that it changes behaviour. If scaleX is NaN and scaleY is not, the result is currently scaleY, but with the proposed change it would be NaN.