Font fallback

[jonleighton]

This implements more complete support for font fallback, see #17267.

r? @glennw @mbrubeck

---

This change is 

[highfive]

Heads up! This PR modifies the following files:

• @emilio: components/layout/text.rs

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[jdm]

This looks amazing! \o/

[glennw]

Reviewed 17 of 23 files at r1.
Review status: 17 of 23 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

components/gfx/platform/freetype/font.rs, line 102 at r1 (raw file):

            FT_New_Memory_Face(lib, bytes.as_ptr(), bytes.len() as FT_Long, face_index, &mut face)
        } else {
            let filename = CString::new(&*template.identifier).expect("filename contains NUL byte!");

One of the reasons for making FT always load via memory here was so that the layout threads would never block on a synchronous file load. The idea was that the font cache thread would responsible for providing these. I'm not sure if that's really relevant or feasible, but that was the original idea, I think.

---

components/gfx/text/util.rs, line 156 at r1 (raw file):

    // https://en.wikipedia.org/wiki/Plane_(Unicode)#Supplementary_Ideographic_Plane
    if unicode_plane(codepoint) == 2 {

nit: could just be unicode_plane(codepoint) == 2 and remove the true / false here, unless there's more conditions to come in the future.

---

Comments from Reviewable

[glennw]

Partial review only so far - I'll try to work through the trickier bits today or tomorrow. Will need @mbrubeck to take a look at some of the text files that I'm not familiar with too.

[glennw]

Reviewed 6 of 23 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/layout/text.rs, line 218 at r1 (raw file):

                let (mut start_position, mut end_position) = (0, 0);
                for (byte_index, character) in text.char_indices() {
                    if !character.is_control() {

Should get @mbrubeck to check this.

---

Comments from Reviewable

[glennw]

Reviewed the remaining files, they look good to me. I think it'd be worth getting @mbrubeck to do a quick pass over this too, as I'm not very up to date on text / font handling in Servo. My main question is related to the thread the font load occurs on (mentioned above).

[jonleighton]

One of the reasons for making FT always load via memory here was so that the layout threads would never block on a synchronous file load. The idea was that the font cache thread would responsible for providing these. I'm not sure if that's really relevant or feasible, but that was the original idea, I think.

Thanks for the explanation. I note that the windows and macos implementations end up calling into platform APIs in this situation, do you know of calling those APIs results in any disk I/O?

Without this change I observed a big slowdown on Linux where the bytes of all the fallback fonts would be passed over the ipc-channel. The slowdown was mostly due to the CPU cost of serialization and deserialization. Performance was a lot better when I tried the serde_bytes crate, so that could be an option here (actually I think it is worth using serde_bytes regardless, but it may make performance acceptable if we want to keep all the I/O in the font cache thread).

One thing it is worth pointing out, is that by the time a layout thread does the file read, the file has already been read by the font cache thread. Therefore, we might hope that the data is already in an OS cache and it might be that no actual disk reads need to occur. I don't know enough about how this works to make any firm statements about that though. Another thing to say is that I'm testing this on a laptop with an SSD hard drive, so I'm probably less likely to notice disk I/O slowdowns.

Do you have any suggestions about how I could/should investigate this further so we can decide what the best approach is?

[jonleighton]

I should add: when I looked into the bytes-over-IPC thing, I realised that the font code had originally been developed before a multiprocess architecture was implemented. Therefore, at the time, those bytes were not getting serialized or sent to a different process. Then, the multiprocess architecture came along and converted everything to IPC channels. I thought that perhaps it wasn't intentionally decided that the bytes should go over IPC, but instead just a consequence of how the code had evolved. It just seemed quite surprising to me to do this, but it might be that it's actually fine if we use serde_bytes.

[glennw]

You're exactly right - the original font code was built before any consideration for multi-process / ipc-channel, and serializing the bytes over the ipc channel was a side effect of that.

I think it's fine to move the file read into the font contexts at this stage - this is certainly a big improvement on what we have now.

We can re-consider the design of how / where we fetch the bytes as a separate issue for later - it's a big topic by itself.

There's no other blockers from me, but it would be good to get @mbrubeck to do a quick pass over this before merging - he has much more experience with the Servo text / layout code than me.

[mbrubeck]

@bors-servo r+

Looks good to me. Thanks for this!

[bors-servo]

📌 Commit e15d3e1 has been approved by mbrubeck

[bors-servo]

⌛ Testing commit e15d3e1 with merge b278ee4...

[bors-servo]

💔 Test failed - mac-dev-unit

[mbrubeck]

error: unused import: `std::borrow::ToOwned`
 --> components/gfx/platform/macos/font_list.rs:6:5
  |
6 | use std::borrow::ToOwned;
  |     ^^^^^^^^^^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`

[jonleighton]

nit: could just be unicode_plane(codepoint) == 2 and remove the true / false here, unless there's more conditions to come in the future.

Done

Should get @mbrubeck to check this.

This wasn't actually 100% necessary for the fallback to work, though it's useful in terms of not doing unnecessary fallbacks for non-printing characters. I've split it out into a separate commit which gives a better explanation of what it does.

We can re-consider the design of how / where we fetch the bytes as a separate issue for later - it's a big topic by itself.

I've added a comment to the code linking to this discussion.

error: unused import: std::borrow::ToOwned

Fixed

[mbrubeck]

@bors-servo r+

[emilio]

@bors-servo r=emilio,mbrubeck

[bors-servo]

📌 Commit 3025a26 has been approved by emilio,mbrubeck

[bors-servo]

⌛ Testing commit 3025a26 with merge 824f403...

[bors-servo]

💔 Test failed - mac-rel-wpt4

[jonleighton]

o_O we managed to crash rustc:

rustc(40975,0x700000e13000) malloc: *** error for object 0x1: pointer being freed was not allocated

[jdm]

@bors-servo retry

• #20796

[bors-servo]

⌛ Testing commit 3025a26 with merge 77dcc67...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio,mbrubeck
Pushing 77dcc67 to master...