-
Notifications
You must be signed in to change notification settings - Fork 14
Avoid parsing subtable if coverage check fails #320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This extracts the primary coverage table (along with any suitable caches) into a field on `SubtableInfo` so that we can check coverage before attempting to parse the subtable. The glyph id, coverage index, coverage metadata and external cache are then passed to the subtable apply function in a new `ApplyState` parameter. This avoids some buffer indexing and reparsing the primary coverage table. This attempts to clean up and unify some of the caching code paths along with laying some groundwork for future optimizations (like #260). HR benchmarks seem to show a reasonable gain on non-Latin shaping as a bonus.
|
I see a 6% slowdown on Roboto, although speedup on heavier Arabic fonts: |
| } | ||
|
|
||
| pub struct ApplyState<'a> { | ||
| pub first_glyph: GlyphId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be u32 at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a u32 underneath. I don't have a preference on the actual type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's good to know. I was under the impression that it's 16bit, and hence a headache to update for beyond-64k.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A while back we added GlyphId16 specifically for 16-bit glyphs and changed our common GlyphId to be 32-bit.
oof, the Roboto benches are so temperamental :( I'll see if I can figure out why. |
|
Patch generally looks fine to me. My only concern is that we're getting more and more distanced from HB in some of the trickiest part of the code. I'm afraid I'll have to defer to you porting any changes in subtable instantiation or caching in the future. |
This extracts the primary coverage table (along with any suitable caches) into a field on
SubtableInfoso that we can check coverage before attempting to parse the subtable. The glyph id, coverage index, coverage metadata and external cache are then passed to the subtable apply function in a newApplyStateparameter. This avoids some buffer indexing and reparsing the primary coverage table.This attempts to clean up and unify some of the caching code paths along with laying some groundwork for future optimizations (like #260).
HR benchmarks seem to show a reasonable gain on non-Latin shaping as a bonus.