fontations icon indicating copy to clipboard operation
fontations copied to clipboard

[read-fonts] Inefficient codegen for field offsets

Open nicoburns opened this issue 3 months ago • 3 comments

Description of issue

read-fonts code for reading blocks of fields seems to follow a pattern where the getter for each field recursively calls into the getter for the previous fields. For example consider the following code from the HEAD table (https://github.com/googlefonts/fontations/blob/main/read-fonts/generated/generated_head.rs#L701):

impl HeadMarker {
    pub fn version_byte_range(&self) -> Range<usize> {
        let start = 0;
        start..start + MajorMinor::RAW_BYTE_LEN
    }

    pub fn font_revision_byte_range(&self) -> Range<usize> {
        let start = self.version_byte_range().end;
        start..start + Fixed::RAW_BYTE_LEN
    }

    pub fn checksum_adjustment_byte_range(&self) -> Range<usize> {
        let start = self.font_revision_byte_range().end;
        start..start + u32::RAW_BYTE_LEN
    }

    pub fn magic_number_byte_range(&self) -> Range<usize> {
        let start = self.checksum_adjustment_byte_range().end;
        start..start + u32::RAW_BYTE_LEN
    }

    pub fn flags_byte_range(&self) -> Range<usize> {
        let start = self.magic_number_byte_range().end;
        start..start + Flags::RAW_BYTE_LEN
    }

    pub fn units_per_em_byte_range(&self) -> Range<usize> {
        let start = self.flags_byte_range().end;
        start..start + u16::RAW_BYTE_LEN
    }
}

units_per_em_byte_range calls into flags_byte_range calls into magic_number_byte_range calls into checksum_adjustment_byte_range calls into font_revision_byte_range which calls into version_byte_range.

This leads to N + (N - 1) + (N - 2) + ... function calls for N fields. In this case there are actually 17 fields, so that's 153 function calls - just to read the header of the HEAD table!

Notes

  • Some "blocks of fields" contain variable-length fields or optional fields which would require this structure. However, in this case all fields are fixed size, and the offset could easily be computed by the code generator.
  • In cases where there are optional or variably sized fields, it is often only some of the fields, and the code generator could be taught to be aware of this and only add a dependency where that is the case.
  • This likely gets optimised out at higher opt levels (although there are so many function calls that I wouldn't be completely confident in that)
  • Nevertheless this will be putting stress on the type-checker, borrow-checker and the optimiser, and I suspect this may be one contributor to read-fonts's poor compile times

nicoburns avatar Sep 13 '25 22:09 nicoburns

Compile-time aside, I assumed these all get inlined and simplified. Can we mark the constant ones as such even?

behdad avatar Sep 13 '25 23:09 behdad

They probably do get inlined, but it's possible that these kinds of patterns are at least partly responsible for the very long compile times since the compiler has to do all the work.

LaurenzV avatar Sep 14 '25 08:09 LaurenzV

My assumption while writing this (and I did some godbolt'ing at the time) was that this would all be optimized away, but it's definitely possible it is hurting compile times. I'd have no problem with an implementation that kept a running tally and just used literals, if someone wanted to do that.

cmyr avatar Sep 15 '25 17:09 cmyr