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

OSX: Add internal cache for UI system fonts as well #2497

Closed
wants to merge 2 commits into from

Conversation

csomor
Copy link
Contributor

@csomor csomor commented Aug 26, 2021

see #19191

@csomor csomor changed the title OSX add internal cache for UI system fonts as well OSX: Add internal cache for UI system fonts as well Aug 26, 2021
@csomor csomor added the macOS Specific to Cocoa macOS port label Aug 26, 2021
@csomor csomor requested a review from vadz August 26, 2021 19:03
Copy link
Contributor

@vadz vadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This should definitely help, but if it matters, it could be improved further.

I also think we could use just a std::vector<wxCFRef<CTFontRef>> (or even a std::array if we wanted the really optimal solution) instead of a map because CTFontUIFontType is just a small integer which practically begs to be used as an index into the vector.

src/osx/carbon/font.cpp Outdated Show resolved Hide resolved

wxCFRef<CTFontRef> ctfont;

auto item = s_UIFontCache.find(uifont);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we care about efficiency that much here, but using find() followed by emplace() does the search in the map twice and the typical thing to do to avoid it is to use lower_bound() to get the iterator either for the object that we want or for insertion later.

However this might be irrelevant because of my more global comment about using a vector instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, yes, I'm not really fluent on these unfortunately ...

Comment on lines +301 to +302
SetFont(ctfont);
m_info.InitFromFont(ctfont);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still involves calling CTFontCopyGraphicsFont() and doing everything wxNativeFontInfo::InitFromFontDescriptor() does, even for the cached fonts. I haven't done any profiling, so perhaps all this is "free" compared to CTFontCreateUIFontForLanguage() itself, but perhaps we should cache the entire wxFont objects to avoid doing all this every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's why I wrote Andy in the Trac report, to please test things, because if this is expensive, then indeed my first idea of caching wxFontRefData would be unavoidable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if there is any doubt, we should go with caching wxFont[RefData] and use a wxModule to clean it up on GUI shutdown, it's slightly more code, but really not difficult. I can do it, if you'd like?

Copy link
Contributor Author

@csomor csomor Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if there is any doubt, we should go with caching wxFont[RefData] and use a wxModule to clean it up on GUI shutdown, it's slightly more code, but really not difficult. I can do it, if you'd like?

that would be great, thank you, actually I'd cache the wxFontDataRefs in the wxFont::wxFont(wxOSXSystemFont font), not only the ones used in settings.mm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll do it. I'd still be grateful for any testing, however.

Co-authored-by: VZ <vz-github@zeitlins.org>
@csomor
Copy link
Contributor Author

csomor commented Aug 26, 2021

Thanks! This should definitely help, but if it matters, it could be improved further.

I also think we could use just a std::vector<wxCFRef<CTFontRef>> (or even a std::array if we wanted the really optimal solution) instead of a map because CTFontUIFontType is just a small integer which practically begs to be used as an index into the vector.

you're right, thank you, I didn't look at the value range of this type.

@vadz
Copy link
Contributor

vadz commented Aug 26, 2021

Closing in favour of #2499.

@vadz vadz closed this Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macOS Specific to Cocoa macOS port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants