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.
Created attachment 186645 [details] Patch
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 ?
Created attachment 186652 [details] Patch, v2 - cairo success check syntax.
(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.
Comment on attachment 186652 [details] Patch, v2 - cairo success check syntax. Clearing flags on attachment: 186652 Committed r141908: <http://trac.webkit.org/changeset/141908>
All reviewed patches have been landed. Closing bug.