-
Notifications
You must be signed in to change notification settings - Fork 721
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
perf: Add a fast path for GetValue #13848
Conversation
@@ -265,6 +265,37 @@ private void Dispose(bool disposing) | |||
|
|||
ValidatePropertyOwner(property); | |||
|
|||
// As a performance optimization, avoid creating the property details and try get the value in this fast path. | |||
if ((propertyDetails ??= _properties.FindPropertyDetails(property)) is null) |
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.
Interesting optimization! Let's see what the build says.
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.
@Youssef1313 Looks like the build is globally passing, though there are some unit tests that are not passing.
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.
@Youssef1313 could you take a look at those tests? Thanks!
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.
@jeromelaban I bypassed this optimization for attached properties as that's all what the failing tests about.
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.
@Youssef1313 so you'd need to change the test, is that what you mean?
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 didn't change the test. Instead, I modified the code to avoid the new optimization when the property is attached. However, looks like some other tests are still failing :/
0112ea9
to
9643610
Compare
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-13848/index.html |
This is no longer needed given the work in #18001 |
Draft for now as I don't want this to get merged without a clear demonstration of an improvement (if any)
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
Copilot Summary
🤖 Generated by Copilot at 0112ea9
This pull request improves the efficiency and correctness of dependency property handling in Uno Platform. It optimizes the
GetValue
method inDependencyObjectStore.cs
to avoid unnecessary allocations and checks, and fixes the inheritance configuration of properties inDependencyPropertyDetailsCollection.cs
.PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Run
results.Other information
Internal Issue (If applicable):