Bug 97993 – [Chromium] Introduce caches for HarfBuzzShaper

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

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 166412 [details]
Patch

[Kenichi Ishibashi]

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.

[Gyuyoung Kim]

Comment on attachment 166412 [details]
Patch

Attachment 166412 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14107009

[Kenichi Ishibashi]

Created attachment 166424 [details]
Patch

[Tony Chang]

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.

[Kenichi Ishibashi]

Created attachment 166595 [details]
Patch

[Kenichi Ishibashi]

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.

[Tony Chang]

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.

[Kenichi Ishibashi]

Created attachment 166769 [details]
Patch for landing

[Kenichi Ishibashi]

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.

[WebKit Review Bot]

Comment on attachment 166769 [details]
Patch for landing

Clearing flags on attachment: 166769

Committed r130231: <http://trac.webkit.org/changeset/130231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

(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.