Bug 97281 – [Chromium] Use harfbuzz-ng by default on Linux

• Component: Platform
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

As of r129175, I think harfbuzz-ng and HarfBuzzShaper are ready to use by default.

- Layout tests results are slightly differ from old harfbuzz, but most of them are improved.
- Page cycler intl2 results are bit regressed, but we should not value the result too much.

Detailed explanations will follow.

[Kenichi Ishibashi]

Here is the LayoutTests results:
https://docs.google.com/open?id=0B6NYyLPujP4TNmJjS0JmbWVKRlU

Summary of results (only complex text related tests):

- Looks improved.
fast/text/cg-vs-atsui.html
fast/text/complex-text-opacity.html
fast/text/atsui-negative-spacing-features.html
fast/text/atsui-spacing-features.html
fast/text/international/hindi-spacing.html
fast/text/international/text-spliced-font.html
fast/text/international/thai-line-breaks.html
platform/chromium-linux/fast/text/international/arabic-vertical-offset.html
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html

- Looks fine.
fast/text/drawBidiText.html
fast/text/international/arabic-justify.html
fast/text/international/hebrew-vowels.html
Tests under svg/W3C-I18N/
svg/W3C-SVG-1.1-SE/text-intro-05-t.svg
svg/W3C-SVG-1.1/text-intro-05-t.svg
svg/text/bidi-text-query.svg
svg/text/text-intro-05-t.svg
transforms/2d/hindi-rotated.html

- Just need rebaselines.
fast/text/international/khmer-selection.html
platform/chromium-linux/svg/text/text-with-geometric-precision.svg

[Kenichi Ishibashi]

Page cycler intl2 summary (20 iterations):

- hb-old

milliseconds: 61405
mean per set: 3070.25
mean per page: 102.34
timer lag: 11499.00
timer lag per page: 19.17

- hb-ng

milliseconds: 64953
mean per set: 3247.65
mean per page: 108.26
timer lag: 11933.00
timer lag per page: 19.89

hb-old a bit fast, but we shouldn't value it too much. Below is a quote from an email from behdad@ about the result:
<quote>
One thing concerns me regarding both these numbers, and LayoutTests.  Looks like all these tests are running against the WinXP set of fonts which are extremely incomplete in their OpenType features.  In particular, Times New Roman from XP doesn't have GDEF or GPOS.  So, we end up hitting a lot of fallback code that is rarely exercised these days.  So, IMO, the numbers are a bit misleading and irrelevant.  With modern fonts I expect new HB to perform better than old HB.
</quote>

See also http://crbug.com/139920.

[Kenichi Ishibashi]

Hi Tony,

I'd like ask your opinion about using harfbuzz-ng by default on Linux. I put the current status and summary. What do you think?

[Evan Martin]

Tony asked me what I think.

1) It'd be nice to eliminate all this old code, so I'm tempted to say we should do it, BUT

2) Why do we even have performance tests if we're not going to believe the results?  Behdad's comment means that either:
2a) The tests are using fonts that don't match what normal users have; or
2b) Normal users are going to get 5% slower.

It would be nice to understand which of those is actually true.  I think 2b is probably fine but we should flip the flag knowing that we're losing performance if that's what it takes.

[Tony Chang]

Can you re-run the perf tests using a different font for both harfbuzz-old and harfbuzz-ng?  For example, you could replace Times New Roman as the default font with "serif" to get the fontconfig default (looks like DejaVu on my system).  You could also try using the ChromeOS fonts (Droid, I think).

We can find out if our users have Times New Roman installed or not using UMA. It would be nice to know how many users are using Times New Roman vs something else.

[Evan Martin]

To clarify: I'm not even in a position of influence on this project anymore, so this is just my opinion.  And my opinion is that it's totally fine to lose 5% of performance if it means deleting a bunch of crufty code that doesn't quite work right.

[Kenichi Ishibashi]

Hi Evan, Tony,

Thank you so much for your inputs. I'll re-run page cycler tests using different font next Monday. I'll also check UMA to have ideas to answer Evan's question. (I'm not sure how I can see UMA, but I guess I can ask someone to see it)

[Jungshik Shin]

> 2a) The tests are using fonts that don't match what normal users have

I responded to the quoted comment of Behdad in the email thread among him, bashi and me, but bashi only quoted Behdad's first comment. 

The answer is not that simple. It all depends on what portion of our users have Arial and Times New Roman (a very old version dating from 2000) as included in mscorettf package. If the majority of our users have those old Arial and Times New Roman, our perf bot test environment is realistic. Even if that's the case, Arial and Times New Roman as included in mscorettf covers only Arabic and Hebrew among complex scripts tested by intl2 page_cycler tests. So, Thai and Indic script pages should not be affected by what Behdad mentioned.  It means that the overall perf regression should come from Arabic/Hebrew portion alone and it's actually more worrisome because the perf regression will be more noticeable for Arabic/Hebrew pages. 

If that's the case, we do now have a better alternative (Arimo, Tinos) to Arial/Times New Roman (dating from 2000) and we can evangelize using those fonts in place of Arial/Times New Roman. 

If the majority of users do not have Arial / Times New Roman (from 2000), Arabic and Hebrew will be rendered with modern fonts and hb-ng's fallback code (which is triggered only when appropriate GSUB/GPOS tables are absent) will not be used.  In that case, our buildbot does not reflect the reality very well and we don't have to worry about this. 

> Can you re-run the perf tests using a different font for both harfbuzz-old and harfbuzz-ng?  
> For example, you could replace Times New Roman as the default font with "serif" to get the fontconfig > default (looks like DejaVu on my system).  You could also try using the ChromeOS fonts (Droid, I think).

ChromeOS fonts corresponding to Arial and Times New Roman are Arimo and Tinos. Just adding a fontconfig substitution directive alone would not work because we require the exact name match. Fortunately for Arimo and Tinos, we already hardcode them to be equivalent to Arial and Times New Roman. So, what's necessary is to uninstall mscorettf package and to add a fontconfig line to make them (Arimo, Tinos) equivalent to Arial and Tinos. 
local.conf file in fontconfig ebuild for ChromeOS has entries necessary for that.

[Jungshik Shin]

> And my opinion is that it's totally fine to lose 5% of performance if it means deleting a bunch of 
> crufty code that doesn't quite work right.

I also agree to the above  despite what I wrote about Arabic/Hebrew.  The correctness is more important than ~5% perf gain for an unknown percentage of users (who have 12-yr old version of Arial and Times New roman).

[Kenichi Ishibashi]

Jungshik,

I'm sorry for not quoting your reply. I should mentioned it. And thank you for the detailed follow-ups.

I ran page cycler intl2 tests using alternative fonts(Arimo and Tinos):

- hb-old
milliseconds	67738
mean per set	3386.90
mean per page	112.90
timer lag	13158.00
timer lag per page	21.93

- hb-ng
milliseconds	72671
mean per set	3633.55
mean per page	121.12
timer lag	13308.00
timer lag per page	22.18

Looks like ToT is somewhat degraded (not sure, there might be another reason on my machine). Anyway, it seems that the performance loss is irrelevant to which fonts are used.

We should improve the performance, but I'd like attach importance to correctness at this time. I and behdad will address the performance as an ongoing work.

If it makes sense, I'll create a patch to make hb-ng by default.

[Tony Chang]

A 7% regression seems like a lot.  I bet there's some low hanging fruit.  Can we profile/optimize first then revisit the question of enabling by default?

I think if we switch now, there will be no incentive to make things faster.

[Behdad Esfahbod]

(In reply to comment #11)
> A 7% regression seems like a lot.  I bet there's some low hanging fruit.  Can we profile/optimize first then revisit the question of enabling by default?

Ok, that is indeed cause for concern.  In testing HarfBuzz alone I have not been able to reproduce this.  So, my gut feeling is that it's coming from the webkit integration layer.

That said, here's how we can move forward:

1) Confirm whether my results are valid.  Ie. whether we can reproduce the slowdown in HarfBuzz along (with old vs new backends) on someone else's testing (bashi?).

2) Updating to ICU 50 should give harfbuzz-ng some speedup.  Jungshik, any update on that?  Alternatively, since we are not relying on the normalization features of hb-ng in Chrome yet, maybe we can disable the normalizer for some speedup.

3) Good old profiling.  We may be doing something differently in the callbacks for example.  It's hard to know what's going on right now.

 
> I think if we switch now, there will be no incentive to make things faster.

While that's generally true, I don't think that would be the case here.  We're removing code that has been unmaintained for many years, and replacing with something that my fulltime job to work on, and is also used in GNOME and Firefox.  It's a major part of my job for the next two years to improve ChromeOS's text rendering, and I know bashi also has similar plans.

BTW, there's also memory savings to be had by the switch.  Checkout: 

  http://goo.gl/woyty

Someone should confirm that we observe this in Chrome.

[Behdad Esfahbod]

As another example of why switching over sooner than later is a good idea: Myanmar seems to be broken with old HarfBuzz but works fine with new.  This website for example:

  http://mm.myanmars.net/

(Uses a webfont.)

[Kenichi Ishibashi]

(In reply to comment #11)
> A 7% regression seems like a lot.  I bet there's some low hanging fruit.  Can we profile/optimize first then revisit the question of enabling by default?
> 
> I think if we switch now, there will be no incentive to make things faster.

I'm trying to profile with --single-process as http://www.chromium.org/developers/profiling-chromium-and-webkit suggests. It seems that fonts for some scripts (Thai, Devenagari, etc) aren't available with --single-process so I'm looking for a better way to profile complex text path rendering.

[Behdad Esfahbod]

(In reply to comment #14)
> (In reply to comment #11)
> > A 7% regression seems like a lot.  I bet there's some low hanging fruit.  Can we profile/optimize first then revisit the question of enabling by default?
> > 
> > I think if we switch now, there will be no incentive to make things faster.
> 
> I'm trying to profile with --single-process as http://www.chromium.org/developers/profiling-chromium-and-webkit suggests. It seems that fonts for some scripts (Thai, Devenagari, etc) aren't available with --single-process so I'm looking for a better way to profile complex text path rendering.

Sounds weird.  Did you make any progress here?

[Behdad Esfahbod]

Tony, Evan, given the discussion, can we get an updated opinion from you two?

[Tony Chang]

My opinion is still the same. I would like to see some profiling/optimization before switching.  We don't have to get back to the same speed, you just need to show me that you're interested in making things faster (so far, it seems very hand-wavy).

[Evan Martin]

Unfortunately --single-process turns off a lot of code, including some of the code related to complex text.  (As I recall it's because we have to run a separate subprocess to make fontconfig calls as it has global data structures and the Chrome UI thread also can make fontconfig calls.)

[Behdad Esfahbod]

bashi, since getting profiling seems to be challenging, can you make sure we're not doing excessive work?  Ie. count the number of face and font creations as well as shape calls for old / new harfbuzz?  I'm suspecting that we're doing something wrong.

[Behdad Esfahbod]

FWIW, I did some fresh measurements with hb-shape with WinXP and Win7 Times New Roman and long Arabic text, and I see solid improvements in new HB.  Here's my test command:

$ time ./hb-shape ~/fonts/WinXP/times.ttf --text-file test-file --output-file /dev/null --no-glyph-names --shaper {ot|old}

(choose one shaper)

I performed two tests, using text files having:

  * 10,000 lines of the same 24k-byte Persian text,

  * 1000,000 lines of the same 10-byte Persian text,

In both cases, for WinXP times.ttf I see new HB faster than old by about 20%, and with Win7 times.ttf I see it 35% faster.  Given the Arabic numbers we are observing in page cycler, I'm sure we're doing something wrong in the webkit integration patch.

[Tony Chang]

(In reply to comment #20)
> 
> In both cases, for WinXP times.ttf I see new HB faster than old by about 20%, and with Win7 times.ttf I see it 35% faster.  Given the Arabic numbers we are observing in page cycler, I'm sure we're doing something wrong in the webkit integration patch.

Great. We should fix this before turning on harfbuzz-ng.

[Kenichi Ishibashi]

HarfBuzzShaper(hb-ng) do more work than ComplexTextController(hb-old) for correct shaping (e.g. positioning and dividing text run). I doubt we are doing something wrong. At least I'm sure we don't create extra hb_font and hb_face objects. Anyway I agree with we need to fix performance issue. I'm swampwed other tasks now, but I will try make time for this.

[Jungshik Shin]

(In reply to comment #12)

> 2) Updating to ICU 50 should give harfbuzz-ng some speedup.  Jungshik, any update on that?  

ICU 50 hasn't yet been released :-) It's supposed to be released on Oct 31st. 

> Alternatively, since we are not relying on the normalization features of hb-ng in Chrome yet, maybe we can disable the normalizer for some speedup.

Have we tried that to see if it helps?

[Kenichi Ishibashi]

(In reply to comment #23)

> > Alternatively, since we are not relying on the normalization features of hb-ng in Chrome yet, maybe we can disable the normalizer for some speedup.
> 
> Have we tried that to see if it helps?

I haven't. I'll try. BTW, normalization on HarfBuzzShaper and ComplexTextController is a bit different.
hb-old: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp&exact_package=chromium&type=cs&l=55

hb-ng: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp&type=cs&l=162

[Kenichi Ishibashi]

(In reply to comment #24)
> > Have we tried that to see if it helps?

Not calling normalizedCharacters() gains 2%, but '\t', '\n', and U+00a0 are displayed with square boxes. These characters should be replaced with spaces in WebKit as normalizedCharacters() did. We can't remove this conversion and get the gain.

[Behdad Esfahbod]

Just to rule out some possibility: what do you get if you use hb_unicode_funcs_get_empty() instead of hb_icu_get_unicode_funcs()?  Results should most be wrong, but I want to see the perf characteristics.

[Kenichi Ishibashi]

The current performance of page cycler intl2 tests (measured by using WebKit r130840).

- hb-old

iterations	5
pages	30
milliseconds	19620
mean per set	3924.00
mean per page	130.80
timer lag	3278.00
timer lag per page	21.85

iterations	20
pages	30
milliseconds	68783
mean per set	3439.15
mean per page	114.64
timer lag	12589.00
timer lag per page	20.98

- hb-ng

iterations	5
pages	30
milliseconds	20097
mean per set	4019.40
mean per page	133.98
timer lag	3134.00
timer lag per page	20.89

iterations	20
pages	30
milliseconds	70054
mean per set	3502.70
mean per page	116.76
timer lag	11702.00
timer lag per page	19.50

hb-ng is slightly slow. Tony, do you think we should make further improvement?

[Kenichi Ishibashi]

(In reply to comment #26)
> Just to rule out some possibility: what do you get if you use hb_unicode_funcs_get_empty() instead of hb_icu_get_unicode_funcs()?  Results should most be wrong, but I want to see the perf characteristics.

Here is the result.

iterations	5
pages	30
milliseconds	19376
mean per set	3875.20
mean per page	129.17
timer lag	3083.00
timer lag per page	20.55


iterations	20
pages	30
milliseconds	65751
mean per set	3287.55
mean per page	109.59
timer lag	12176.00
timer lag per page	20.29

It actually became faster, but it also implied that the bottlenecks are in my WebKit side..

BTW, here are some notes about performance regression.
- hb-old implementation does NFC normalization in advance. This contributes to performance. But it causes wrong selection behavior because the character index will be mismatched between the original TextRun and normalized character sequence. For example, you can see an wrong behavior on fast/text/atsui-kerning-and-ligatures.html (Please try to select ameÌt from left to right) hb-ng implementation fixed this issue, but we needed to add further checks whether the font to be used actually has the glyphs for combining characters.
- hb-old implementation directly calls Skia APIs. On the other hand, hb-ng implementation fills glyphs and positions into GlyphBuffer and calls port-independent function to draw glyphs. This is a design choice to make hb-ng implementation be Skia-independent. However, this might affect the performance.

[Tony Chang]

This looks much better!  We're down to a 2-2.5% regression.  Yes, let's switch.  Thank you for profiling and finding some low hanging fruit first.

[Kenichi Ishibashi]

Created attachment 168301 [details]
Patch

[Kenichi Ishibashi]

Hi Tony,

Thank you so much for helping this transition.

I'm preparing a patch to switch. Should I also update test expectations in this patch? Or just adding suppressions in TestExpectations and then updating expectations by using garden-o-matic is ok?

[Kenichi Ishibashi]

Created attachment 168313 [details]
Add temporary expectations

[Tony Chang]

Comment on attachment 168313 [details]
Add temporary expectations

Suppressing in TestExpectations and rebaselining using garden-o-matic is fine.  Please watch the perf bots and be ready to update the expectations file if they go red.

[Kenichi Ishibashi]

(In reply to comment #33)
> (From update of attachment 168313 [details])
> Suppressing in TestExpectations and rebaselining using garden-o-matic is fine.  Please watch the perf bots and be ready to update the expectations file if they go red.

I see.  I'll keep watch on perf bots after landing. I'll cq+ later so that I can work together with non-PST gardener.

I guess perf bots may go red. If the regression rate is within a expected range(~2%), I'll ask perf sheriffs whether we can update the perf expectation.

[WebKit Review Bot]

Comment on attachment 168313 [details]
Add temporary expectations

Attachment 168313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14262313

[WebKit Review Bot]

Comment on attachment 168313 [details]
Add temporary expectations

Clearing flags on attachment: 168313

Committed r131134: <http://trac.webkit.org/changeset/131134>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.