941470 - explicitly enabling common ligatures and kerning leads to slower text rendering

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

Based on the results from a text perf benchmark I'm working on, it appears that explicitly enabling common ligatures and kerning is slower than simply using the default, which should show the same results.

Arial with defaults:
http://people.mozilla.org/~jdaggett/textbench/textbenchrun.html?test=basic_generic_standard_fonts_arial_simpletext_en

Arial with kerning and ligatures explicitly enabled:
http://people.mozilla.org/~jdaggett/textbench/textbenchrun.html?test=basic_generic_standard_fonts_arial_kerning_and_ligatures_simpletext_en

Steps:

Load each URL in Nightly, then refresh each page.  Result - on my machine with the defaults the test takes 28ms, with kerning/ligatures explicitly set it takes 37.5ms.

Maybe there's a simple explanation here but I thought it would be a good idea to log this in case this is something easy to fix.

Comment 1

[Jonathan Kew [:jfkthame]]

This is a known harfbuzz issue; see http://lists.freedesktop.org/archives/harfbuzz/2013-October/003817.html. In principle we could work around it by adding shape-plan caching at the Gecko gfxHarfBuzzShaper level, but I think it's best done within harfbuzz itself.

Behdad, any chance you could look at merging that patch sometime soon?

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: [harfbuzz] cache shape plan even if user features are set, provided all user features are global

For reference, here's the plan-caching patch proposed for harfbuzz, ready to apply to the mozilla tree. John, if you want to experiment with this you should be able to measure a substantial improvement for testcases where any explicit feature settings are used.

Comment 3

[Behdad Esfahbod]

Will take a look.  Thanks.

Comment 4

[Behdad Esfahbod]

John, do you have more details about your textbench?

Comment 5

[John Daggett (:jtd)]

(In reply to Behdad Esfahbod from comment #4)
> John, do you have more details about your textbench?

Full testsuite is here:
http://people.mozilla.org/~jdaggett/textbench/

Sample results for FF19 - FF28 Nightly, Windows 7
http://pastebin.mozilla.org/3644805

To compare results:

1. Open up a given results URL
2. Paste in the comparison results URL and hit enter

I also sent you more details via email.

Comment 6

[Behdad Esfahbod]

Upstream now.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: update harfbuzz to latest upstream commit (f47b9219...), to pick up plan-caching for user features and misc fixes

This includes the caching fix mentioned above, as well as a couple of other fixes (see harfbuzz commit log for details).

Comment 8

[John Daggett (:jtd)]

Reftest layout/reftests/text/graphite-03a.html is failing on all platforms with this patch.

Comment 9

[Behdad Esfahbod]

Humm.  I have no clue why that may be.  Leaving to Jonathan.

Comment 10

[John Daggett (:jtd)]

tryserver builds with patch:
https://tbpl.mozilla.org/?tree=Try&rev=37a03fa8ca6a

w/o patch:
https://tbpl.mozilla.org/?tree=Try&rev=4c04210be142

textbench run comparison:
http://pastebin.mozilla.org/3707623

Results look consistently faster with patch (including the disabled feature tests) with one exception.  For some reason the generic korean font tests under Windows 7 run roughly 8% slower:

extended_langs_generic_fonts_serif_simpletext_ko      77.8ms[1.7%]  84.4ms[1.1%]  108.5% ▼
extended_langs_generic_fonts_sansserif_simpletext_ko  78.3ms[0.5%]  84.0ms[1.8%]  107.3% ▼
extended_langs_generic_fonts_monospace_simpletext_ko  79.3ms[0.5%]  86.9ms[1.8%]  109.6% ▼

Test steps:
Clean profile. Run once and restart browser.  Run again, use those results.

Single test URL:
http://people.mozilla.org/~jdaggett/textbench/textbenchrun.html?test=extended_langs_generic_fonts_serif_simpletext_ko

Note: various other tests show slowdowns but in those cases the variance seems to explain the difference.  The korean tests above were the only slowdowns that don't appear to be simply testing noise.

Comment 11

[Behdad Esfahbod]

A slowdown is definitely not expected.

Comment 12

[Jonathan Kew [:jfkthame]]

It's because of this addition to hb-ot-tag.cc:

+  {"ksw",	HB_TAG('K','R','N',' ')},	/* S'gaw Karen */
+/*{"ksw",	HB_TAG('K','S','W',' ')},*/	/* S'gaw Karen (OpenType spec and SIL fonts) */

(Note the commented-out entry, as well as the one actually used.)

Previously, "ksw" was not explicitly listed in HB's table. So lang="ksw" in the testcase was activating OpenType langSys 'KSW' because of the default behavior of hb_ot_tag_from_language() ("Assume it's ISO-639-3 and upper-case and use it"), and so the language-specific feature in Padauk was used as expected. But the new mapping to 'KRN' means this no longer works.

Possible fixes:

(a) Change Harfbuzz to use the second of the mappings above for "ksw", rather than the first. Why was that mapping used - is this in Microsoft's font? (What is the OpenType spec mentioned there? I don't see S'gaw Karen in the list at http://www.microsoft.com/typography/otspec/languagetags.htm.)

(b) Accept the changed OpenType behavior (though it breaks OpenType Padauk - but we don't care much about that as we use Graphite by default) and change the test to == instead of !=.

(c) Fix the test by adding -moz-font-language-override: "KSW" so that we continue to get the SIL langSys used, rather than the new Harfbuzz mapping.

Behdad, can you comment on (a)? Is there a strong reason for Harfbuzz to use this (unofficial?) mapping, or should we revert that?

If we don't want to do (a), then I'll put up a patch implementing (c) so as to avoid the test failure.

Comment 13

[Behdad Esfahbod]

Thanks for checking.  Roozbeh has an action item to send me details of the contentious cases (there are a couple more) and I'll follow up with OpenType + SIL people.  CC'ing roozbeh.

Comment 14

[Behdad Esfahbod]

In the mean time feel free to send me a patch to upstream to bring previous behavior back.

Comment 15

[Behdad Esfahbod]

Change upstreamed.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: update harfbuzz to latest upstream commit (205bf834...), to pick up plan-caching for user features and misc fixes

Updated patch to pick up the commit from upstream. Pushed a try job to check that the test is happier now: https://tbpl.mozilla.org/?tree=Try&rev=e67914b47c5f.

Comment 17

[John Daggett (:jtd)]

Patch looks fine as long as the reftest passes and we figure out the reason for the Korean generic font regression on Windows.

Comment 18

[Jonathan Kew [:jfkthame]]

How consistently reproducible is this "regression"? When I repeatedly run your http://people.mozilla.org/~jdaggett/textbench/textbenchrun.html?test=extended_langs_generic_fonts_serif_simpletext_ko example locally, the times I get vary pretty widely; for example, just hitting Reload ten times in quick succession on that page, I got:

extended_langs simpletext ko 56ms sd: 0.6% serif
extended_langs simpletext ko 42.4ms sd: 1.3% serif
extended_langs simpletext ko 64ms sd: 2.7% serif
extended_langs simpletext ko 47.8ms sd: 2.0% serif
extended_langs simpletext ko 40.9ms sd: 0.6% serif
extended_langs simpletext ko 66ms sd: 2.2% serif
extended_langs simpletext ko 44.4ms sd: 1.2% serif
extended_langs simpletext ko 54ms sd: 3.1% serif
extended_langs simpletext ko 51ms sd: 2.9% serif
extended_langs simpletext ko 40.6ms sd: 1.0% serif

With that much fluctuation between successive runs, I'm not sure what to make of it...

Comment 19

[Jonathan Kew [:jfkthame]]

Tryserver says reftests are all green now:
https://tbpl.mozilla.org/?tree=Try&rev=e67914b47c5f

Comment 20

[John Daggett (:jtd)]

Comment on attachment 8341436 [details] [diff] [review]
update harfbuzz to latest upstream commit (205bf834...), to pick up plan-caching for user features and misc fixes

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> How consistently reproducible is this "regression"? When I repeatedly run
> your
> http://people.mozilla.org/~jdaggett/textbench/textbenchrun.
> html?test=extended_langs_generic_fonts_serif_simpletext_ko example locally,
> the times I get vary pretty widely; for example, just hitting Reload ten
> times in quick succession on that page, I got:

I was running the full suite twice after browser start and using that result.  Running repeatedly suffers from the fact that the word cache will be in a different state each time and with a single test you're more likely to be affected by word cache dumps (i.e. the word cache clears itself when it reaches a given count).

Since this is primarily a harfbuzz change, I compared the results of running the tests without the word cache enabled (i.e. set gfx.font_rendering.wordcache.charlimit to 0).  Based on this, I think these changes are fine, no anomoly for Korean appears.

Results:
http://pastebin.mozilla.org/3713895

One thing that's puzzling is that on Windows, running with kerning and ligatures
explicitly enabled seems to be much *faster* (?!?) than with no features explicitly enabled (this is the same behavior as before, it's not related to patch changes).

Default:
http://people.mozilla.org/~jdaggett/textbench/textbenchrun.html?test=basic_generic_standard_fonts_serif_simpletext_en

With kerning/ligatures explicitly enabled:
http://people.mozilla.org/~jdaggett/textbench/textbenchrun.html?test=basic_generic_standard_fonts_serif_kerning_and_ligatures_simpletext_en

With the word cache disabled, the default test takes 87ms while the test with kerning/ligatures explicitly enabled takes 56ms.  Under OSX the results are pretty much the same.  I'm guessing this is since most generic fonts use CoreText for shaping.  We can fix that here or push it to another bug.

Comment 21

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #20)

> Under OSX the results are pretty much the same.

i.e. the timings with and without the features specified is pretty much the same.

Comment 22

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dda95b5cb528

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #20)
> With the word cache disabled, the default test takes 87ms while the test
> with kerning/ligatures explicitly enabled takes 56ms.

That's curious, indeed.

>  Under OSX the results
> are pretty much the same.  I'm guessing this is since most generic fonts use
> CoreText for shaping.  We can fix that here or push it to another bug.

Yes, with default font prefs, most generics will be AAT fonts that we shape via Core Text. And we don't have font feature support hooked up for these, so specifying features in the CSS isn't likely to have much effect. So that's an entirely separate matter.

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> (In reply to John Daggett (:jtd) from comment #20)
> > With the word cache disabled, the default test takes 87ms while the test
> > with kerning/ligatures explicitly enabled takes 56ms.
> 
> That's curious, indeed.

On further thought, this makes perfect sense.

It's because the default serif font on Windows has kerning that involves the space glyph. When you enable kerning *explicitly*, we ignore the word cache and just shape each textrun in its entirety (as per bug 761442) in a single harfbuzz call. But when kerning is only *implicitly* enabled, we still allow the word cache to be used, which means we scan the text looking for spaces and then shape each word separately - even though they're all "too long to be cached", so we end up getting no word-cache benefit; instead we just suffer the overhead.

Or in other words, because of the kerning-with-space in Times New Roman, disabling the word cache has no impact on the explicit-kerning testcase, but it significantly hurts the no-features case. Note that it also has an impact on the actual rendering (in theory, at least); with the cache enabled, those word/space-boundary kerns won't be applied.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Ioana (away)]

Comparison between the 11/21 Fx28 Nightly and 02/10 Fx28 Beta - both runs on my Windows 7 32bit:
http://pastebin.mozilla.org/4257089

Comparison between what's on the URL section and a run on my Fx28 Beta (Win7 32bit):
http://pastebin.mozilla.org/4257101