Glyph struct optimisation

[benoitkugler]

Hi,

I was wondering how you would consider a breaking change cleaning and optimizing the shaping.Glyph struct.
There are two changes I would like to make :

• replacing the fields XAdvance and YAdvance by an unique Advance field. XAdvance and YAdvance are never used together : XAdvance is for horizontal text, YAdvance for vertical text. With this change, we could also simplify our internal logic and remove switches on the direction.

• as once suggested by Chris, we could reduce the size of the RuneCount and GlyphCount fields to use uint8. In practice, theses numbers never exceed (say) 10, so using a full int is a bit of a waste.

[whereswaldon]

So I think the two primary considerations here are:

• Is there a use-case in which you want access to both the X and Y advance of a glyph? The one that springs to mind is glyph caching. If you cache glyph data, you previously only needed to worry about the size, and now you need to know the size and axis. Do other text shaping technologies simplify down to just one advance in returned glyph data? I'm fairly sure that harfbuzz offers both.
• As far as shrinking the counts, I agree. I'm not aware of any use-case in which a single glyph represents more than 16 runes, and I'm unsure of the upper bound on how many glyphs can occur in a single shaping cluster. That's probably font-defined. It's tempting to say we can give each 4 bits of a byte, but since the GlyphCount has a less rigid bound, we should probably do two bytes. Unless we could pack the RuneCount into 4 bits of some other field and access it via some method.

[benoitkugler]

Is there a use-case in which you want acess to both the X and Y advance of a glyph? The one that springs to mind is glyph caching. If you cache glyph data, you previously only needed to worry about the size, and now you need to know the size and axis. Do other text shaping technologies simplify down to just one advance in returned glyph data? I'm fairly sure that harfbuzz offers both.

I'm pretty sure harfbuzz only sets the advance of the shaping direction, because the advance (in general) is influenced by surrounding glyphs and opentype layout. As opposed to glyph Width and Height.
In my understanding, the advance of a glyph may vary between shaping anyway and should not be cached.

[benoitkugler]

As far as shrinking the counts, I agree. I'm not aware of any use-case in which a single glyph represents more than 16 runes, and I'm unsure of the upper bound on how many glyphs can occur in a single shaping cluster. That's probably font-defined. It's tempting to say we can give each 4 bits of a byte, but since the GlyphCount has a less rigid bound, we should probably do two bytes. Unless we could pack the RuneCount into 4 bits of some other field and access it via some method.

Packing is a nice idea, I'll double check we can use other fields.

[whereswaldon]

I'm pretty sure harfbuzz only sets the advance of the shaping direction, because the advance (in general) is influenced by surrounding glyphs and opentype layout. As opposed to glyph Width and Height. In my understanding, the advance of a glyph may vary between shaping anyway and should not be cached.

They probably are contextual, but it's worth noting that Harfbuzz offers both in the output:

typesetting/shaping/shaping.go

Line 135 in 9645b81

glyphs[i].YAdvance = fixed.I(int(t.buf.Pos[i].YAdvance)) >> scaleShift

Maybe the expectation is that you'll only access one of those fields though.

[benoitkugler]

I'm pretty sure harfbuzz only sets the advance of the shaping direction, because the advance (in general) is influenced by surrounding glyphs and opentype layout. As opposed to glyph Width and Height. In my understanding, the advance of a glyph may vary between shaping anyway and should not be cached.

They probably are contextual, but it's worth noting that Harfbuzz offers both in the output:

typesetting/shaping/shaping.go

Line 135 in 9645b81

glyphs[i].YAdvance = fixed.I(int(t.buf.Pos[i].YAdvance)) >> scaleShift
Maybe the expectation is that you'll only access one of those fields though.

You're right. It not obvious at a first glance of the harfbuzz package that only one of the field is used at a time. I'll need to double check that.
(Having said that, I fail to understand how YAdvance could be useful in horizontal layout, since there already is the Yoffset field to perform cross alignement.)

[benoitkugler]

Reading Harfbuzz documentation, we find

hb_position_t x_advance : how much the line advances after drawing this glyph when setting text in horizontal direction.

hb_position_t y_advance : how much the line advances after drawing this glyph when setting text in vertical direction.

That seems to strongly hint at only one field being used at a time

[benoitkugler]

Note : the field Mask set by Harfbuzz is a uint32, with only 3 bit used : that leaves plenty of room to pack rune and glyph counts.

[benoitkugler]

They probably are contextual, but it's worth noting that Harfbuzz offers both in the output:

typesetting/shaping/shaping.go

Line 135 in 9645b81
glyphs[i].YAdvance = fixed.I(int(t.buf.Pos[i].YAdvance)) >> scaleShift

Maybe the expectation is that you'll only access one of those fields though.

For the record, I've carefully checked the Harfbuzz codebase and I'm now sure that YAdvance is only set in vertical direction, XAdvance in horizontal direction.
I have also added the following check in the harfbuzz test suite, without triggering any error :

for _, pos := range buffer.Pos {
	if buffer.Props.Direction.isHorizontal() {
		tu.Assert(t, pos.YAdvance == 0)
	} else {
		tu.Assert(t, pos.XAdvance == 0)
	}
}

[andydotxyz]

Just adding a comment here that I think I placed somewhere else relating to this discussion:
Is optimisations worth a breaking change?

If the answer is yes then we should coordinate release versioning very careflly around this so that libraries have a chance to make usage changes before some developers execute "update all" on their apps and get compile errors.

[benoitkugler]

I think this change would be great if we started from scratch, but I can live with the current state.

I note that we could probably propose a transition path : adding a new field Advance, and two methods GlyphCount() and RuneCount(), and deprecate the fields {X,Y}Advance, RuneCount, GlyphCount. Then remove it in a new version.
Note sure it is worth the hassle.

[whereswaldon]

Our text shaping results are quite memory-heavy, especially when maintaining a shaper cache for application use. Shrinking the memory footprint by doing this stuff would have a significant impact on that footprint. The current struct is at least 70 bytes, and may be more due to padding and alignment requirements. The discussed changes to drop one advance field saves 4 bytes, and dropping the rune/glyph count by bit-packing them into the mask can save another 16 bytes on 64-bit systems. The Glyph.ClusterIndex could also be explicitly made into a int32 or uint32 to save another 4 bytes without a meaningful loss in capability. We do not expect to shape individual paragraphs consisting of more than 2 billion runes.

That would enable a total size reduction of 24 bytes (need to check how the padding plays out), resulting in a savings of 34% of the memory footprint.