Skip to content

[read-fonts] Track running offset in FontData#1583

Open
dfrg wants to merge 1 commit intomainfrom
track-offset
Open

[read-fonts] Track running offset in FontData#1583
dfrg wants to merge 1 commit intomainfrom
track-offset

Conversation

@dfrg
Copy link
Copy Markdown
Member

@dfrg dfrg commented Aug 5, 2025

... 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

... 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.
@dfrg
Copy link
Copy Markdown
Member Author

dfrg commented Aug 5, 2025

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

@behdad
Copy link
Copy Markdown
Contributor

behdad commented Aug 5, 2025

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.

@dfrg
Copy link
Copy Markdown
Member Author

dfrg commented Aug 5, 2025

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

Copy link
Copy Markdown
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no objections, but can you help me understand exactly what checks this helps us elide?

Presumably there's a double bounds-check happening that you hope this will let us condense, but it isn't jumping out to me where exactly that is occurring...

@dfrg
Copy link
Copy Markdown
Member Author

dfrg commented Aug 6, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants