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

[Windows] Optimize getting default font size and font family values #22782

Merged

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Jun 2, 2024

Description of Change

I've been profiling my "complex grid" case again (see #21787) and I noticed that getting DefaultFontFamily and DefaultFontSize is costly on Windows because it requires interop operations on every access of FontManager.DefaultFontFamily and FontManager.DefaultFontSize.

Interestingly, on Android it's just a simple constant.

Moreover, on IOS the value is cached.

But on Windows, the values are read over and over again.

There are two paths possible for Windows here:

  1. Cache the two values. This would be very easy but it might turn out to be wrong (this PR)
  2. Cache DefaultFontFamily and DefaultFontSize but listen to changes (it appears ContentControlThemeFontFamily is basically static; see [Windows] Optimize getting default font size and font family values #22782 (comment)).

Performance impact

image

-> ~%79 improvement

Issues Fixed

Contributes to #21787

@MartyIX
Copy link
Contributor Author

MartyIX commented Jun 2, 2024

(Perhaps @Youssef1313 would know if this PR can work given his recent work with RegisterPropertyChangedCallback. Sorry for pinging if not or not interested.)

@MartyIX MartyIX requested a review from Foda June 2, 2024 08:59
@jsuarezruiz
Copy link
Contributor

/azp run

@jsuarezruiz jsuarezruiz added platform/windows 🪟 area-fonts Custom fonts and Font related API's labels Jun 3, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Youssef1313
Copy link
Member

(Perhaps @Youssef1313 would know if this PR can work given his recent work with RegisterPropertyChangedCallback. Sorry for pinging if not or not interested.)

Hi @MartyIX

I don't recall doing stuff related to RegisterPropertyChangedCallback in MAUI. So, I don't have enough context to review, unfortunately :(

@MartyIX
Copy link
Contributor Author

MartyIX commented Jun 3, 2024

@Youssef1313 Thank you for your reply! Yes, though you filed unoplatform/uno#15437 issue. This PR is actually more related to WinUI than to MAUI. But perhaps I got it wrong.

@Youssef1313
Copy link
Member

Youssef1313 commented Jun 3, 2024

@MartyIX Ah, in that case, it's not yet clear to us exactly why inheritance changes don't fire the callback. However, if there is nothing MAUI-specific here, I don't see how these RegisterPropertyChangedCallback can ever get called in this specific case. To my understanding, Application.Current.Resources.RegisterPropertyChangedCallback(Control.FontFamilyProperty, OnPropertyChanged); means, if FontFamilyProperty changed on Application.Current.Resources, then call OnPropertyChanged. However, ResourceDictionary isn't actually Control in the first place. So, I doubt the callback can get called.

@MartyIX
Copy link
Contributor Author

MartyIX commented Jun 3, 2024

@MartyIX [..] So, I doubt the callback can get called.

Yes, I could not make it fire so I think it's not correct. Would you have an idea of a different approach or how to make this work?

I tend to think that what I try to achieve here is not possible (at the moment).

@Youssef1313
Copy link
Member

Yes I don't think WinUI exposes an API to listen for changes in ResourceDictionary. But maybe there could be a way to somehow reduce the number of calls to DefaultFontFamily / DefaultFontSize

@MartyIX
Copy link
Contributor Author

MartyIX commented Jun 3, 2024

Yes I don't think WinUI exposes an API to listen for changes in ResourceDictionary. But maybe there could be a way to somehow reduce the number of calls to DefaultFontFamily / DefaultFontSize

So I found https://github.com/microsoft/microsoft-ui-xaml/blob/2a60e27c591846556fa9ec4d8f305afdf0f96dc1/dxaml/xcp/dxaml/themes/generic.xaml#L14 and https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.media.fontfamily.xamlautofontfamily?view=winrt-22621.

Then one can see many instances of {ThemeResource ContentControlThemeFontFamily} in WinUI's source code.

ThemeResource changes with theme but otherwise it's static (AFAIK) (resolved once), not a dynamic resource.

It's hard to imagine that default font or font size changes with a theme change, so just caching the values once seems easy and correct. Fully correct would be to cache values after theme changes which seems reasonably easy to do as well.

Would you agree with the assessment or not?

@Youssef1313
Copy link
Member

@MartyIX I guess users can do something like Application.Current.Resources["ContentControlThemeFontFamily"] = new FontFamily("CustomFont"); to change the resource value, but the question is "Is this a scenario that should be supported?"

@MartyIX
Copy link
Contributor Author

MartyIX commented Jun 3, 2024

@MartyIX I guess users can do something like Application.Current.Resources["ContentControlThemeFontFamily"] = new FontFamily("CustomFont"); to change the resource value, but the question is "Is this a scenario that should be supported?"

Good point.

Given my previous comment, it should not lead to UI changes because WinUI uses {ThemeResource ContentControlThemeFontFamily}. The dictionary value can still change but what would be the purpose of it (if I'm right in the analysis above it would be mostly ignored).

The good thing is that I can try this change in a sandbox project to see what would happen.

@MartyIX MartyIX force-pushed the feature/2024-06-02-Windows-Fonts-perf branch from cf8ef0a to ab8e7da Compare June 4, 2024 17:14
@MartyIX MartyIX marked this pull request as ready for review June 4, 2024 18:23
@MartyIX MartyIX requested a review from a team as a code owner June 4, 2024 18:23
@MartyIX MartyIX requested review from jfversluis and rmarinho June 4, 2024 18:23
@Foda
Copy link
Member

Foda commented Jun 5, 2024

I think just caching the value is fine. If this becomes a big issue for people we can either introduce a flag to disable the cache, or revert the PR. But I think 99% of users will benefit from this so we should merge it.

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow merged commit 9d71d32 into dotnet:main Jun 6, 2024
47 of 49 checks passed
@MartyIX MartyIX deleted the feature/2024-06-02-Windows-Fonts-perf branch June 6, 2024 15:52
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants