CoreText: Faster OTC font loading #232
Conversation
This commit vastly simplifies the way .otc parsing is done in font-kit by initializing a font from a descriptor and not copying the buffer. It also makes Handle capable of storing an already loaded font, as what happens currently is that we first have a Font which we turn into a Handle (losing track of the Font that was used to create it) only to recreate a font from that same handle (and reparsing the font file in the process). Less waste is good. Meanwhile, on this branch parsing Iosevka takes about 30ms and most of that time is spent.. well.. parsing the font I guess?
src/handle.rs
Outdated
| /// Retrieves a handle to the font object. | ||
| /// | ||
| /// May return None if inner object is not of type `T` or if this handle does not contain a native font object. | ||
| pub fn native_as<T: 'static>(&self) -> Option<&T> { |
There was a problem hiding this comment.
Hrm. Any reason this isn't called as_native?
There was a problem hiding this comment.
Ah, no. There's no particular reason for that, I'll change it. Funnily enough, in doc comments I've referred to it as as_native. Oh well. :p
src/lib.rs
Outdated
| @@ -144,5 +144,5 @@ pub mod source; | |||
| #[cfg(feature = "source")] | |||
| pub mod sources; | |||
|
|
|||
| mod matching; | |||
There was a problem hiding this comment.
Could this change be split out into a different PR?
src/loader.rs
Outdated
| ref bytes, | ||
| font_index, | ||
| } => Self::from_bytes((*bytes).clone(), font_index), | ||
| match handle { | ||
| Handle::Memory { bytes, font_index } => Self::from_bytes((*bytes).clone(), *font_index), | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| Handle::Path { | ||
| ref path, | ||
| font_index, | ||
| } => Self::from_path(path, font_index), | ||
| Handle::Path { path, font_index } => Self::from_path(path, *font_index), |
There was a problem hiding this comment.
Do you mind removing unrelated reformatting?
| Font { | ||
| core_text_font, | ||
| font_data, | ||
| font_data: FontData::Unavailable, |
There was a problem hiding this comment.
What the repercussions of no longer having this font data?
There was a problem hiding this comment.
Ha, good catch; Font implements Deref to [u8] which will panic if there's no data associated: https://github.com/servo/font-kit/blob/master/src/loaders/core_text.rs#L784
Granted, this can still happen on master if a font has no path.
Also, Font::copy_font_data will return None instead of font contents, but that at least does not panic in surprising ways. I'd say that if the panic and implicit deref to [u8] poses a problem, one can always turn to using Font::copy_font_data, which gives a chance to avoid a panic.
There was a problem hiding this comment.
On the other hand, this PR is already Semver-breaking with a change to Loader::from_native_font, so we might as well get rid of the Deref implementation?
| Font::from_core_text_font_no_path(core_text::font::new_from_CGFont( | ||
| &core_graphics_font, | ||
| 16.0, | ||
| )) |
There was a problem hiding this comment.
Well, it kinda is; Font::from_core_text_font was always trying to load data from disk, which is not necessary as we already have a font.
| @@ -160,7 +161,8 @@ impl Font { | |||
|
|
|||
| /// Creates a font from a native API handle. | |||
| #[inline] | |||
| pub unsafe fn from_native_font(native_font: NativeFont) -> Font { | |||
| pub unsafe fn from_native_font(native_font: &NativeFont) -> Font { | |||
| let native_font = native_font.clone(); | |||
There was a problem hiding this comment.
Is this cloning the font data or just the handle here?
There was a problem hiding this comment.
This one is a bit more involved as we're going through COM (https://github.com/servo/dwrote-rs/blob/master/src/font.rs#L44, https://github.com/servo/dwrote-rs/blob/master/src/font_face.rs#L31), but I'd also say that this is bumping just the refcounts. So we should be good.
| // We make an in-memory copy of the underlying font data. This is because the native font | ||
| // does not necessarily hold a strong reference to the memory backing it. | ||
| let freetype_face = *freetype_face; |
There was a problem hiding this comment.
Same question effectively here.
There was a problem hiding this comment.
In this case we're copying a pointer, so it's just a handle.
|
Hey, is there anything I can do to push this PR forward? |
| #[cfg(feature = "source")] | ||
| #[test] | ||
| pub fn get_font_data() { |
There was a problem hiding this comment.
Why is this test being removed?
I re-added it locally and it passed.
There was a problem hiding this comment.
What OS are you on? It fails for me on Mac, iirc due to https://github.com/servo/font-kit/pull/232/files#diff-6ba3732b9cf93e63d757c9bbbe03032cb17ed4525b45153ec0217b7df271ebf1R132 which leads to copy_font_data call failing.
There was a problem hiding this comment.
Maybe it's unwise to remove the test. At the very least, we can cfg-gate it.
Background: At Zed we've had users report that certain fonts could take a long time to load (zed-industries/zed#6205). Admittedly these font files are quite big (Iosevka font weights like 300Mb), but I've still went looking and found out that font-kit might sometimes copy excessive amount of memory, as seen in the flamegraph of loading of Iosevka:
This commit does several things:
loadagain.matchingmodule in public interface of a crate so that users don't have to go through Source::select_best_match`.The latter two changes are there so that you don't have to go through the font loading code just to get a font/just to match it. In order to initialize a Handle a font is loaded, but that Font is discarded (and needlessly loaded later) on current master.
Note that I've pulled this commit verbatim from how we're using it in Zed; I made sure tests pass on Windows/Linux CI, but I didn't test on these platforms (as we're only available on Mac for now). The macOS part works fine on production for few days already.
Another reason to have that commit (I guess) is that it vastly improves throughput of examples like
list-fontsandmatch-font(#209). The particular command-line that this user was using runs 10x faster for me with this commit (0.15s vs 1.1s on M1 Max).list-fontscompletes in ~2s on debug build (vs not completing within 10s or so on current master).