fonts: Instantiate system fonts using system font loaders #33747
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
61a561c to
af626c8
Compare
|
dwrote was updated in #33766. |
5 tasks
mukilan
reviewed
Oct 10, 2024
components/fonts/font.rs
Outdated
| pub template: FontTemplateRef, | ||
| pub metrics: FontMetrics, | ||
| pub descriptor: FontDescriptor, | ||
|
|
||
| /// The data for this font. This might be unitialized for system fonts. |
There was a problem hiding this comment.
Suggested change
| /// The data for this font. This might be unitialized for system fonts. | |
| /// The data for this font. This might be uninitialized for system fonts. |
components/fonts/font_store.rs
Outdated
| @@ -121,66 +77,32 @@ impl FontStore { | |||
| /// load was canceled (for instance, if the stylesheet was removed), then do nothing and return | |||
| /// false. | |||
| /// | |||
| /// `data` will be `None` When loading fonts via the `local()` directive in CSS, in which case | |||
There was a problem hiding this comment.
This method doesn't have a data parameter.
| pub use font::*; | ||
| pub use font_context::*; | ||
| pub use font_store::*; | ||
| pub use font_template::*; | ||
| pub use glyph::*; | ||
| use ipc_channel::ipc::IpcSharedMemory; | ||
| pub use shaper::*; | ||
| pub use system_font_service::*; | ||
| use unicode_properties::{emoji, EmojiStatus, UnicodeEmoji}; | ||
|
|
There was a problem hiding this comment.
This documentation seems outdated as we can no longer send FontData over IPC channels directly and we don't support the as_arc method.
There was a problem hiding this comment.
I've updated this documentation to reflect the fact the handle is what is sent across the IPC channel.
| @@ -43,14 +44,29 @@ impl LocalFontIdentifier { | |||
| .map_or(0, |font| font.create_font_face().get_index()) | |||
| } | |||
|
|
|||
| pub(crate) fn read_data_from_file(&self) -> Vec<u8> { | |||
| let font = FontCollection::system() | |||
| pub fn native_font_handle(&self) -> NativeFontHandle { | |||
There was a problem hiding this comment.
Should this be pub(crate)? It is not pub in other platforms.
| system_font_service: Arc<MockSystemFontService>, | ||
| system_font_service_proxy: SystemFontServiceProxy, | ||
| } | ||
| // This currently only works on FreeType platforms as it requires being able ot create |
There was a problem hiding this comment.
Suggested change
| // This currently only works on FreeType platforms as it requires being able ot create | |
| // This currently only works on FreeType platforms as it requires being able to create |
af626c8 to
e422131
Compare
|
Thanks for the review @mukilan. I believe I have addressed all of your comments. |
e422131 to
66fea8a
Compare
mukilan
approved these changes
Oct 10, 2024
66fea8a to
b02fc1e
Compare
|
🔨 Triggering try run (#11274731178) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#11274731178): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (18)
|
|
✨ Try run (#11274731178) succeeded. |
b02fc1e to
c831ea7
Compare
c831ea7 to
effe36e
Compare
|
🔨 Triggering try run (#11280960555) for Android, OpenHarmony |
|
|
System fonts used to be instantiated using the system font loader and this change restores that behavior. In addition, on macOS and FreeType platforms font data for system fonts is loaded using memory mapping. The benefit is that system font loaders typically are able to cache fonts in system memory (using memory mapping, for instance) and we'd like to load them in a the way most compatible with other applications. On my Linux system, this manages to get the overhead of loading a very large font down from 10ms to approximately 1ms. Subsequent runs show even less overhead. We've measured similar gains on macOS systems. Currently, system font data must be loaded into memory manually for canvas and this is unlikely to change even with a switch to `vello`. The use of explicit memmory mapping should help in this case -- though it probably won't be possible to use this properly on macOS and Windows if we ever want to load fonts from TTCs properly. Signed-off-by: Martin Robinson <[email protected]> Co-authored-by: Mukilan Thiyagarajan <[email protected]>
effe36e to
7695f40
Compare
I can't speak to compatibility with other applications, but my understanding is that the caching benefit applies to any non-system font loaders that use memory mapping too. System fonts are just files on the system, and OS page caches will share memory mapped pages across processes. So a font file memory mapped in a Rust process ought to reuse the same physical memory that the system loader (and other processes using fonts) are using. |
|
Yes, I suspect that system fonts are usually memory mapped. This change ensures that when system fonts are loaded they are memory mapped with the same configuration as other applications as much as possible. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
System fonts used to be instantiated using the system font loader and
this change restores that behavior. In addition, on macOS and FreeType
platforms font data for system fonts is loaded using memory mapping. The
benefit is that system font loaders typically are able to cache fonts in
system memory (using memory mapping, for instance) and we'd like to load
them in a the way most compatible with other applications.
On my Linux system, this manages to get the overhead of loading a very
large font down from 10ms to approximately 1ms. Subsequent runs show
even less overhead. We've measured similar gains on macOS systems.
Currently, system font data must be loaded into memory manually for
canvas and this is unlikely to change even with a switch to
vello. Theuse of explicit memmory mapping should help in this case -- though it
probably won't be possible to use this properly on macOS and Windows if
we ever want to load fonts from TTCs properly.
Signed-off-by: Martin Robinson [email protected]
Co-authored-by: Mukilan Thiyagarajan [email protected]
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors