863248 - update harfbuzz to pick up recent performance improvements

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew (:jfkthame)]

There have been some significant perf improvements in upstream harfbuzz recently, particularly for complex fonts but should be beneficial for virtually all fonts with any GSUB/GPOS lookups.

Accordingly, we should take a harfbuzz update soon (within the current cycle).

Initially, I plan to do some testing with e4046080c5d785c8dbf9ec9e3478ab9acc83e479 from upstream, but there may well be additional commits we'll want to include before actually landing a new version into m-c.

Comment 1

[Jonathan Kew (:jfkthame)]

Attachment: update harfbuzz to version 0.9.16 from upstream

Behdad has now tagged a new release, v0.9.16. This includes some major perf improvements (see discussion on the harfbuzz list, especially Khaled and Behdad's timings with Amiri). It's not clear that we'll see a shift in any Talos results, as our word-caching tends to hide the underlying shaping performance, but standalone testing shows it is considerably faster with complex fonts.

Comment 2

[John Daggett (:jtd)]

Looks fine but my one concern is that there's a lot of code related to serialization in this update that seems like dead code from the Mozilla point of view.

How close are we to enabling Harfbuzz for scripts on Windows beyond Arabic/Hebrew?

Comment 3

[Jonathan Kew (:jfkthame)]

(In reply to John Daggett (:jtd) from comment #2)
> Looks fine but my one concern is that there's a lot of code related to
> serialization in this update that seems like dead code from the Mozilla
> point of view.

Yes, we're not using that functionality; but the "dead code" should be harmless, the linker will leave it out of our finished build anyway.

> How close are we to enabling Harfbuzz for scripts on Windows beyond
> Arabic/Hebrew?

Breadth of testing. To really have confidence in that switch, we'd like to be regularly validating harfbuzz against uniscribe behavior for hundreds of thousands (millions?) of text sequences shaped with hundreds (thousands?) of different fonts. The serialization support is one of the pieces of the testing puzzle; we still need to figure out a good solution to managing the massive quantities of test data and reporting results in a useful way.

(Informally, I think both Behdad and I believe harfbuzz is to the point where we could make the switch and it would be fine. But I'm not comfortable with testing that belief by means of simply shipping it to all our users and waiting for bug reports.)

Comment 4

[Behdad Esfahbod]

Jonathan, how about you add a feature in Firefox, such that on 1% of runs, it shapes text with both Uniscribe and HarfBuzz, and reports back if they are different?

Comment 5

[Jonathan Kew (:jfkthame)]

(In reply to Behdad Esfahbod from comment #4)
> Jonathan, how about you add a feature in Firefox, such that on 1% of runs,
> it shapes text with both Uniscribe and HarfBuzz, and reports back if they
> are different?

Interesting idea - I guess we could do something like that to get statistics regarding how often there are discrepancies. But to get actionable feedback that we could use to understand -what- the discrepancies are, and determine whether they're acceptable differences or shaping bugs, we'd need it to report the actual text being shaped - which sounds like a pretty serious privacy issue.

Comment 6

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> (In reply to John Daggett (:jtd) from comment #2)
> > Looks fine but my one concern is that there's a lot of code related to
> > serialization in this update that seems like dead code from the Mozilla
> > point of view.
> 
> Yes, we're not using that functionality; but the "dead code" should be
> harmless, the linker will leave it out of our finished build anyway.

Are you sure that's true across all platforms?  My understanding is the MS linker is fairly agressive but is the same true for the Android/B2G tool chain?

Comment 7

[xunxun]

(In reply to John Daggett (:jtd) from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #3)
> > (In reply to John Daggett (:jtd) from comment #2)
> > > Looks fine but my one concern is that there's a lot of code related to
> > > serialization in this update that seems like dead code from the Mozilla
> > > point of view.
> > 
> > Yes, we're not using that functionality; but the "dead code" should be
> > harmless, the linker will leave it out of our finished build anyway.
> 
> Are you sure that's true across all platforms?  My understanding is the MS
> linker is fairly agressive but is the same true for the Android/B2G tool
> chain?

Maybe to use -ffunction-sections -fdata-sections -Wl,--gc-sections ?

Comment 8

[Jonathan Kew (:jfkthame)]

(In reply to John Daggett (:jtd) from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #3)
> > (In reply to John Daggett (:jtd) from comment #2)
> > > Looks fine but my one concern is that there's a lot of code related to
> > > serialization in this update that seems like dead code from the Mozilla
> > > point of view.
> > 
> > Yes, we're not using that functionality; but the "dead code" should be
> > harmless, the linker will leave it out of our finished build anyway.
> 
> Are you sure that's true across all platforms?  My understanding is the MS
> linker is fairly agressive but is the same true for the Android/B2G tool
> chain?

Duh....forget what I said. Of course, it -won't- leave out the serialization stuff in our build. We're putting harfbuzz into a shared library (either libxul or gkmedias), so the linker can't remove anything that is reachable from the public API, as users of the shared lib might call any and all of those functions.

I guess on Windows it may strip the serialization stuff because we're not listing it in symbols.def, but on other platforms it'll definitely be present.

I'll investigate how much compiled code this amounts too. I suppose if it seems too much, we could ask Behdad if he's willing to have an HB_NO_SERIALIZATION option that omits this from the build.

Comment 9

[Jonathan Kew (:jfkthame)]

(Of course, in principle we could do that locally - but I don't want us to be maintaining local patches on top of harfbuzz, if we can avoid it.)

Comment 10

[Jonathan Kew (:jfkthame)]

Hah - it's simpler than that. Note that the patch here does NOT add hb-buffer-serialize.cc to our Makefile.in. Hence, it doesn't become part of our build. (Note that we don't use harfbuzz's own Makefile.am.)

Note that prior to this update, there was actually -more- "dead" serialization code present in our build, because some of it was in hb-buffer.cc. With the move to a separate file, it becomes trivial for us to omit it.

So FWIW, comparing the final compressed size of the Fennec APK before/after the update here, the overall difference ends up being negligible (less than 10 bytes!). If we -do- add in hb-buffer-serialize.cc, that increases the APK size by less than 1500 bytes.

Comment 11

[Behdad Esfahbod]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Behdad Esfahbod from comment #4)
> > Jonathan, how about you add a feature in Firefox, such that on 1% of runs,
> > it shapes text with both Uniscribe and HarfBuzz, and reports back if they
> > are different?
> 
> Interesting idea - I guess we could do something like that to get statistics
> regarding how often there are discrepancies. But to get actionable feedback
> that we could use to understand -what- the discrepancies are, and determine
> whether they're acceptable differences or shaping bugs, we'd need it to
> report the actual text being shaped - which sounds like a pretty serious
> privacy issue.

Right...  Even getting font info may be helpful as  we can go test the offending fonts ourselves.

Comment 12

[John Daggett (:jtd)]

Comment on attachment 740340 [details] [diff] [review]
update harfbuzz to version 0.9.16 from upstream

Ok, great, r+ given that.

Comment 13

[Jonathan Kew (:jfkthame)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61d2bdb83bbb

Comment 14

[Ed Morley [:emorley]]

Backed out for failures in kerning-sanity-check-nokern.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=22404267&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f0fe66ee2a

Comment 15

[Jonathan Kew (:jfkthame)]

Oops, sorry - that test wasn't in my tree yet when I was initially testing this.

Note that it's an UNEXPECTED-PASS. Turns out it's correct. The comment in the reftest manifest that "XP version of Arial doesn't contain kerning data" is not quite accurate; there -is- kerning, but it's only provided in the legacy <kern> table, not as a <kern> feature in <GPOS>. As such, it was unaffected by enabling/disabling the kern feature, and that's why the test failed on XP.

The harfbuzz update provides better support for this case: upstream commit 038c98f6866fe1177b04bd2ae3bb461b2f0fd1ed (Feb 15th) made application of the legacy <kern> table respond to the same feature control as modern <GPOS>-based kerning. Hence the test now passes even on XP.

So I'll re-land this with that fails-if annotation removed from the reftest manifest.

Comment 16

[Jonathan Kew (:jfkthame)]

With fingers crossed...
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4fd9ec8b0d0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4fd9ec8b0d0