Skip to content

Performance fixes for GSUB/GPOS dumps#5547

Merged
iorsh merged 4 commits intofontforge:masterfrom
iorsh:subtable_search
Jun 26, 2025
Merged

Performance fixes for GSUB/GPOS dumps#5547
iorsh merged 4 commits intofontforge:masterfrom
iorsh:subtable_search

Conversation

@iorsh
Copy link
Copy Markdown
Contributor

@iorsh iorsh commented Mar 31, 2025

This PR improves the performance of saving large OpenType GPOS/GSUB tables.

@khaledhosny, on my machine the performance of saving Amiri font improved roughly from 68s to 10s, with comparable HarfBuzz metrics view (#5522) improvement. This is still suboptimal, but some Amiri kerning features are loaded as subtable with 6.8M (yes, six million) kerning pairs. I don't know what causes it, as the original font doesn't have anything like that.

Type of change

  • Non-breaking change

@khaledhosny
Copy link
Copy Markdown
Contributor

khaledhosny commented Apr 2, 2025

This is still suboptimal, but some Amiri kerning features are loaded as subtable with 6.8M (yes, six million) kerning pairs. I don't know what causes it, as the original font doesn't have anything like that.

FontForge has limited support for class based kerning, it supports only certain value record formats, and for the rest it explodes the class kerning into kern pairs:

fontforge/fontforge/parsettfatt.c

Lines 556 to 562 in 072208f

/* Accept forms both with and without device tables */
if ( (vf1==0x0008 || vf1==0x0088) && vf2==0x0000 )
isv = 1; /* Top to bottom */
else if ( (vf1==0x0004 || vf1==0x0044) && vf2==0x0000 )
isv = 0; /* Left to right */
else
isv = 2; /* Can't optimize, store all 8 settings */

fontforge/fontforge/parsettfatt.c

Lines 662 to 675 in 072208f

} else { /* This happens when we have a feature which is neither 'kern' nor 'vkrn' we don't know what to do with it so we make it into kern pairs */
int k,m;
subtable->per_glyph_pst_or_kern = true;
for ( i=0; i<c1_cnt; ++i) {
for ( j=0; j<c2_cnt; ++j) {
readvaluerecord(&vr1,vf1,ttf);
readvaluerecord(&vr2,vf2,ttf);
if ( vr1.xadvance!=0 || vr1.xplacement!=0 || vr1.yadvance!=0 || vr1.yplacement!=0 ||
vr2.xadvance!=0 || vr2.xplacement!=0 || vr2.yadvance!=0 || vr2.yplacement!=0 )
for ( k=0; k<info->glyph_cnt; ++k )
if ( class1[k]==i )
for ( m=0; m<info->glyph_cnt; ++m )
if ( class2[m]==j )
addPairPos(info, k,m,subtable,&vr1,&vr2,stoffset,ttf);

@khaledhosny
Copy link
Copy Markdown
Contributor

khaledhosny commented Apr 2, 2025

This is because KernClass uses an array of 16-bit integers for kerning values (offsets):

fontforge/fontforge/splinefont.h

Lines 593 to 609 in 072208f

typedef struct kernclass {
int first_cnt, second_cnt; /* Count of classes for first and second chars */
char **firsts; /* list of a space separated list of char names */
char **seconds; /* one entry for each class. Entry 0 is null */
/* and means everything not specified elsewhere */
char **firsts_names; // We need to track the names of the classes in order to round-trip U. F. O. data.
char **seconds_names;
int *firsts_flags; // This tracks the storage format of the class in U. F. O. (groups.plist or features.fea) and whether it's a single-character class.
int *seconds_flags; // We also track the name format (@MMK or public.kern).
struct lookup_subtable *subtable;
uint16_t kcid; /* Temporary value, used for many things briefly */
int16_t *offsets; /* array of first_cnt*second_cnt entries with 0 representing no data */
int *offsets_flags;
DeviceTable *adjusts; /* array of first_cnt*second_cnt entries representing resolution-specific adjustments */
struct kernclass *next; // Note that, in most cases, a typeface needs only one struct kernclass since it can contain all classes.
int feature; // This indicates whether the kerning class came from a feature file. This is important during export.
} KernClass;

But too fully represent GPOS PairPosFormat2 subtable, it would probably need two valuerecord arrays, and all code that interacts with kerning classes (including the UI) will need to be updated/extended to handle this.

That is one of the severe limitations of FontForge when working with RTL fonts, as it means it basically can’t handle RTL class-based pair positioning as it requires adjusting both the offset and advance width of the glyph.

It might be possible to detect RTL kerning and store and store only one value and special flag that it is RTL so that when writing GPOS back, the value is used for both offset and advance. This might not be perfect, but it will at least handle the common RTL kerning case while not needing too much changes to existing code.

@iorsh
Copy link
Copy Markdown
Contributor Author

iorsh commented Jun 26, 2025

Run bulk test

frank-trampe
frank-trampe approved these changes Jun 26, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@frank-trampe frank-trampe left a comment

Choose a reason for hiding this comment

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

@iorsh is going to run a few more tests, but the code conceptually looks good.

@iorsh
Copy link
Copy Markdown
Contributor Author

iorsh commented Jun 26, 2025

Bulk tests passed with the following environment:

FF_PY_SCRIPT_LINE="f=fontforge.open(argv[1]); f.generate(argv[2], flags='no-FFTM-table')"
FF_OUTPUT_FILE="Output.ttf"

@iorsh iorsh merged commit a7c62ec into fontforge:master Jun 26, 2025
7 checks passed
@iorsh iorsh deleted the subtable_search branch June 26, 2025 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frank-trampe frank-trampe frank-trampe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iorsh @khaledhosny @frank-trampe