| Summary: | [Chromium] Use harfbuzz-ng by default on Linux | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kenichi Ishibashi <bashi> | ||||||
| Component: | Platform | Assignee: | Kenichi Ishibashi <bashi> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | behdad, dglazkov, evan, jshin, tony, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 97993 | ||||||||
| Bug Blocks: | 72984 | ||||||||
| Attachments: |
|
||||||||
|
Description
Kenichi Ishibashi
2012-09-20 19:03:20 PDT
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 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. 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? 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. 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. 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. 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) > 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. > 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).
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. 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. (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. 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.) (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. (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? Tony, Evan, given the discussion, can we get an updated opinion from you two? 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). 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.) 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. 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.
(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. 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. (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? (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 (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. 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. 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? (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. 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. Created attachment 168301 [details]
Patch
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? Created attachment 168313 [details]
Add temporary expectations
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.
(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. 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 Comment on attachment 168313 [details] Add temporary expectations Clearing flags on attachment: 168313 Committed r131134: <http://trac.webkit.org/changeset/131134> All reviewed patches have been landed. Closing bug. |