See https://bugs.webkit.org/show_bug.cgi?id=97281 for the context. I used profiling tool to find which functions impact on performance. Findings are: (1) SkPaint::textToGlyphs() and its callees took 3% of profiling samples. It doubled (compared with hb-old implementation). (2) HarfBuzzShaper::collectHarfBuzzRuns() and its collees took 2% of profiling samples. We can improve the performance by introducing cache for these functions.
Created attachment 166412 [details] Patch
performance_ui_tests (PageCyclerTest.Intl2File) results: https://docs.google.com/spreadsheet/ccc?key=0AqNYyLPujP4TdHZ5MjZIWV9ISVpybDZPUnNfRFc3Nmc It seems that memory consumption of hb-ng with the patch is less than hb-old implementation.
Comment on attachment 166412 [details] Patch Attachment 166412 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14107009
Created attachment 166424 [details] Patch
Comment on attachment 166424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166424&action=review Just some style nits. The ownership of m_glyphCache is a little confusing. It might be more clear if you named m_glyphCache more descriptively in HarfBuzzFontData and HarfBuzzNGFace. Maybe something like m_glyphCacheForFaceCacheEntry? > Source/WebCore/ChangeLog:10 > + - Implement canRenderCombiningCharacterSequence() for ports which use > + HarfBuzzShaper. This function caches the result and will improve the > + performance of HarfBuzzShaper::collectHarfBuzzRuns. Please include performance numbers in the ChangeLog. E.g., this makes the intl2 page cycler X% faster. > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:176 > +#if USE(HARFBUZZ_NG) > +bool SimpleFontData::canRenderCombiningCharacterSequence(const UChar*, size_t) const Which build uses this file? I thought Blackberry and Chromium use the SimpleFontDataSkia.cpp. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:47 > +struct FaceCacheEntry { > + unsigned m_refCount; Can we use RefCounted rather than doing our own ref counting? The HashMap would have RefPtr's holding 1 ref. You could use ref/deref directly and when the ref becomes 1, remove it from the cache (which should cause the destructor to run and delete m_face). > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:68 > + result.iterator->second = WTF::adoptPtr(new FaceCacheEntry); > + result.iterator->second->m_refCount = 0; > + result.iterator->second->m_face = createFace(); I would put this into a static create() method. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceSkia.cpp:179 > + HarfBuzzFontData* hbFontData = new HarfBuzzFontData; > + hbFontData->m_glyphCache = m_glyphCache; I would make a constructor for HarfBuzzFontData that takes m_glyphCache as a parameter.
Created attachment 166595 [details] Patch
Comment on attachment 166424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166424&action=review Thank you for review! >> Source/WebCore/ChangeLog:10 >> + performance of HarfBuzzShaper::collectHarfBuzzRuns. > > Please include performance numbers in the ChangeLog. E.g., this makes the intl2 page cycler X% faster. Added. >> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:176 >> +bool SimpleFontData::canRenderCombiningCharacterSequence(const UChar*, size_t) const > > Which build uses this file? I thought Blackberry and Chromium use the SimpleFontDataSkia.cpp. efl port uses this file. It enables WTF_USE_HARFBUZZ_NG (in Source/cmake/OptionsEfl.cmake). >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:47 >> + unsigned m_refCount; > > Can we use RefCounted rather than doing our own ref counting? The HashMap would have RefPtr's holding 1 ref. You could use ref/deref directly and when the ref becomes 1, remove it from the cache (which should cause the destructor to run and delete m_face). Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:68 >> + result.iterator->second->m_face = createFace(); > > I would put this into a static create() method. Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceSkia.cpp:179 >> + hbFontData->m_glyphCache = m_glyphCache; > > I would make a constructor for HarfBuzzFontData that takes m_glyphCache as a parameter. Done.
Comment on attachment 166595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166595&action=review > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:60 > + FaceCacheEntry(hb_face_t* face) Nit: explicit > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.h:63 > + WTF::HashMap<uint32_t, uint16_t>* m_glyphCacheForFaceCaheEntry; Typo: Cahe -> Cache. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceSkia.cpp:58 > + WTF::HashMap<uint32_t, uint16_t>* m_glyphCacheForFaceCaheEntry; Typo here too.
Created attachment 166769 [details] Patch for landing
Comment on attachment 166595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166595&action=review Thank you for review! >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:60 >> + FaceCacheEntry(hb_face_t* face) > > Nit: explicit Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.h:63 >> + WTF::HashMap<uint32_t, uint16_t>* m_glyphCacheForFaceCaheEntry; > > Typo: Cahe -> Cache. Thanks. Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceSkia.cpp:58 >> + WTF::HashMap<uint32_t, uint16_t>* m_glyphCacheForFaceCaheEntry; > > Typo here too. Done.
Comment on attachment 166769 [details] Patch for landing Clearing flags on attachment: 166769 Committed r130231: <http://trac.webkit.org/changeset/130231>
All reviewed patches have been landed. Closing bug.
Comment on attachment 166769 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=166769&action=review > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:-90 > - Why was this code removed? Looks like after r130231 when this patch was landed, m_glyphToCharacterIndexes doesn't get initialized anymore.
(In reply to comment #13) > Why was this code removed? Looks like after r130231 when this patch was landed, m_glyphToCharacterIndexes doesn't get initialized anymore. Never mind, I found + uint16_t* glyphToCharacterIndexes = currentRun->glyphToCharacterIndexes(); where it actually does get initialized.