Skip to content
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

Re-evaluate the need for DependencyPropertyGenerator #18671

Open
Youssef1313 opened this issue Nov 3, 2024 · 4 comments
Open

Re-evaluate the need for DependencyPropertyGenerator #18671

Youssef1313 opened this issue Nov 3, 2024 · 4 comments
Labels
area/code-generation Categorizes an issue or PR as relevant to code generation area/performance 📈 Categorizes an issue or PR as relevant to performance difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. kind/enhancement New feature or request triage/untriaged Indicates an issue requires triaging or verification

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Nov 3, 2024

What would you like to be added

The internal DependencyPropertyGenerator was originally designed and intended to sacrifice extra memory use in the DependencyObject for a super super fast DP reads. The read through the generator is basically just a backing field read.

Now, with the all the recent DP system performance improvements, it's likely that the regular DP reads are fast enough to not cause any trouble. In that case, it means the generator just adds more memory pressure without much performance benefits.

The performance should be re-evaluated to see if the generator is still needed, or if it's only an overhead without a benefit.

My guess is that now the DP reads should be fast enough that the generator shouldn't be needed.

Extra note: If we need the generator to avoid the boilerplate for declaring DPs, we should then change the way the code is generated to be just like any regular DP.

Why is this needed

Cleanup + Performance

For which platform

All

Anything else we need to know?

The relevant benchmarks are in https://github.com/unoplatform/uno/blob/master/src/SamplesApp/Benchmarks.Shared/Suite/Windows_UI_Xaml_Controls/DependencyPropertyBench/SimpleDPBenchmark.cs

The outcome of the benchmarks at the time of implementing the generator is documented in https://github.com/unoplatform/uno/blob/3304465bbae63aab228cde20365157c74390aa76/doc/articles/uno-development/Internal-DependencyProperty-Generator.md

Those benchmarks are run against FrameworkElement.Width DP. So, we need to run the benchmarks twice. Once with the GeneratedDependencyProperty attribute here

[GeneratedDependencyProperty(
, and once without the attribute by declaring the DP in the normal way.

@Youssef1313 Youssef1313 added kind/enhancement New feature or request triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. area/performance 📈 Categorizes an issue or PR as relevant to performance area/code-generation Categorizes an issue or PR as relevant to code generation labels Nov 3, 2024
@Youssef1313
Copy link
Member Author

In case there is still a big performance gap between the two approaches, it would be good to understand where the time is spent in the DP system and see if there is something we could optimize.

@jeromelaban
Copy link
Member

The reason for the DP generator is to avoid calling into the DP subsystem at all, and particularly avoid doing costly conversions.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Nov 10, 2024

@jeromelaban Yes, but the real reason of not calling into the DP system is performance. So, if the performance difference isn't huge, then that needs to be revisited. If the performance benefit is significant, investigations may lead to actually improving the DP system perf.

Specially for some DPs which are actually not commonly accessed, like FrameworkElement.TagProperty. For that property actually, I'd tend to think it shouldn't use the generated DP even if there is still a performance gap. It's not commonly accessed so in practice the perf difference isn't going to be significant at all. But reducing the object size of FrameworkElement may actually have an impact on applications with very large and complex visual trees.

In short, there is clearly a trade-off between memory and speed here, which I think should be revised in two aspects. First is the need of the generated DP altogether, and second is revising the need for it for specific DPs that are not commonly read (e.g, the TagProperty example for which we are increasing the size of all FrameworkElements with little to no benefit - having smaller FrameworkElements is more important than having fast reads of Tag IMO as Tag isn't read that commonly).

@Youssef1313
Copy link
Member Author

Youssef1313 commented Nov 10, 2024

One other thing that's important to note here:

Accessing a DP when it has default value goes into a code path different from when it was previously assigned a value. So, both of these cases needs to be benchmarked. I think when it's default it should be faster.

For some properties that are even commonly read, it can be common that their value remains as the default value, in which case, it may be better to reduce memory usage if we know that most accesses will go through the fast default value path. So, as there are trade-offs, the idea here is to optimize for the common scenario, which can be different for each DP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code-generation Categorizes an issue or PR as relevant to code generation area/performance 📈 Categorizes an issue or PR as relevant to performance difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. kind/enhancement New feature or request triage/untriaged Indicates an issue requires triaging or verification
Projects
None yet
Development

No branches or pull requests

2 participants