fontations icon indicating copy to clipboard operation
fontations copied to clipboard

[read-fonts] Track running offset in FontData

Open dfrg opened this issue 4 months ago • 4 comments

... rather than slicing the data each time we traverse an offset.

This also currently removes the null check for non-nullable offsets. Note that HB also seems to not check this: https://github.com/harfbuzz/harfbuzz/blob/dbbf6def3b4b33965febd301bb7f7f00ad0856cf/src/hb-open-type.hh#L322

The intent is to remove some overhead when traversing the object graph, particularly for layout subtables.

@behdad would be interesting to see if this has any effect on HR perf by itself

dfrg avatar Aug 05 '25 15:08 dfrg

I'm actually seeing some significantly worse times in the HR bench suite. Swing and a miss?

dfrg avatar Aug 05 '25 15:08 dfrg

I also see 10 to 20% slowdown.

I think it's that now ready every member has more work to do, because the bytes processing didn't get cheaper, it's just that now we're holding onto an extra offset member to pass around.

behdad avatar Aug 05 '25 16:08 behdad

My hope was that inlining + CSE would take care of the redundant additions but that's clearly not happening.

dfrg avatar Aug 05 '25 16:08 dfrg

The idea is to avoid the bounds check due to slicing the font data every time we traverse an offset. We check bounds on everything later on anyway so the first one is unnecessary. This assumption didn't seem to hold up under measurement and actually made things worse, perhaps due to the cost of copying of the larger FontData struct or increased register pressure. I think #1588 buys us significantly more by avoiding a lot of bounds checks so I'll probably end up dumping this one.

dfrg avatar Aug 06 '25 17:08 dfrg