-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize Measure and Arrange #6219
base: main
Are you sure you want to change the base?
Conversation
// Notify parent if our desired size changed (waterfall effect), otherwise return | ||
if (MeasureDuringArrange || DoubleUtil.AreClose(prevSize, desiredSize)) return; | ||
|
||
GetUIParentOrICH(out var parent, out var contentHost); //only one will be returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetUIParentOrICH(out var parent, out var contentHost); //only one will be returned | |
GetUIParentOrContentHost(out var parent, out var contentHost); //only one will be returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, you make the code clearer.
|
||
LayoutTransformData ltd = LayoutTransformDataField.GetValue(this); | ||
LayoutTransformData ltd = LayoutTransformDataField.GetValue(this); | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the brackets?
double eps = (Math.Abs(value1) + Math.Abs(value2) + 10.0) * DBL_EPSILON; | ||
double delta = value1 - value2; | ||
return(-eps < delta) && (eps > delta); | ||
return Math.Abs(value1 - value2) < 10.0 * DBL_EPSILON; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bad news is I tested to replace Math.Abs, but the result will be changed.
In my other PR, I have tried to do it that is similar your code, but I failed. See #5571
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the result be changed essentially or are these just minor details? As described in the comment, the method is not expected to deliver exact results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deeprobin Just minor details.
@@ -83,7 +80,7 @@ public static bool AreClose(double value1, double value2) | |||
/// <param name="value2"> The second double to compare. </param> | |||
public static bool LessThan(double value1, double value2) | |||
{ | |||
return (value1 < value2) && !AreClose(value1, value2); | |||
return value1 < value2 && !AreClose(value1, value2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding ()
makes the meaning clearer.
@deeprobin Please take a look about the error output
|
See #4768 (this does not fix the issue - only supports the performance problem)
Description
In this PR I try to improve the Measure and Arrange method performance-wise. I have also moved certain code blocks to other methods for readability.
Testing
I unfortunately could not test it due to build issues.
Risk
ContextLayoutManager.From (Risk 1/10)
I have reduced the ContextLayoutManager.From calls.
Normally this should fit but I can't confirm this 100%.
DoubleUtil.AreClose changed behavior (Risk 6/10)
I have changed the behavior of DoubleUtil.AreClose. This definitely needs to be tested.
Since this method is called frequently I tried to reduce the corresponding JIT codegen.
I managed to do this, but there is a certain risk, even if according to the summary the method should not return an exact result.