Bug 108941 – [HarfBuzz][Cairo] harfBuzzGetGlyph is slow and hot

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dominik Röttsches (drott)]

Using a performance test similar to PerformanceTests/Layout/hindi-line-layout.html but replacing it with arabic test, we see that 
harfBuzzGetGlyph spends too much time in UTF8 encoding and talking to cairo for glyph lookup.

We should adopt a caching implementation similar to Skia's harfBuzzGetGlyph implementation. The infrastructure for this caching is already anchored in the m_glyphCacheForFaceCacheEntry member of HarfBuzzFace.

[Dominik Röttsches (drott)]

Created attachment 186645 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186645&action=review

> Source/WebCore/ChangeLog:3
> +        [HarfBuzz][Cairo] harfBuzzGetGlyph is slow and hot

If she is hot and slow, you gotta make the move!

> Source/WebCore/ChangeLog:18
> +        Arabic line breaking test, under review in
> +        bug 108948 shows about 58% improvement on my system
> +        with this patch.

Very nice! Any idea how much this would affect the page load of a normal arabic site?

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:105
> +    cairo_scaled_font_t* scaledFont = hbFontData->m_cairoScaledFont;

Why not just make an accessor, it is inlined anyway and accessing m_ looks wrong

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:113
> +        if (CAIRO_STATUS_SUCCESS != cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0))

why not the other way around != CAIRO_STATUS_SUCCESS ?

[Dominik Röttsches (drott)]

Created attachment 186652 [details]
Patch, v2 - cairo success check syntax.

[Dominik Röttsches (drott)]

(In reply to comment #2)

Thanks for taking a look, Kenneth.

> (From update of attachment 186645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186645&action=review

> > Source/WebCore/ChangeLog:18
> > +        Arabic line breaking test, under review in
> > +        bug 108948 shows about 58% improvement on my system
> > +        with this patch.
> 
> Very nice! Any idea how much this would affect the page load of a normal arabic site?

Not yet, one reason is that I don't know what a "normal arabic site" is. I hope I can measure this reliably soon. Also, I plan to have additional improvements for pages that render using the complex font path.

> > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:105
> > +    cairo_scaled_font_t* scaledFont = hbFontData->m_cairoScaledFont;
> 
> Why not just make an accessor, it is inlined anyway and accessing m_ looks wrong

Took that from the Skia implementaiton. I tested this with an accessor and it's not automatically inlined. When I put "inline" explicitly, it's back to normal perfomance. Without "inline" it's down by 5 runs/s. I think putting an accessor and then an explicit inline doesn't look better than accessing the m_* in this case. What do you think?

> > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:113
> > +        if (CAIRO_STATUS_SUCCESS != cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0))
> 
> why not the other way around != CAIRO_STATUS_SUCCESS ?

Inverted.

[WebKit Review Bot]

Comment on attachment 186652 [details]
Patch, v2 - cairo success check syntax.

Clearing flags on attachment: 186652

Committed r141908: <http://trac.webkit.org/changeset/141908>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.