Bug 65688 – [Chromium] Reduce memory consumption of HarfbuzzFace

• Component: Platform
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

We can eliminate multiple allocation of HB_FontRec for the same uniqueID by introducing a cache.

[Kenichi Ishibashi]

Created attachment 102918 [details]
Patch V0

[Evan Martin]

Comment on attachment 102918 [details]
Patch V0

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

I don't understand the font code well enough to know if this is a good idea.
Some questions, at least:

- is the memory use of HarfbuzzFace a problem you've measured?
- why aren't higher-level caches in WebKit over fonts caching this as well?
- since the cache appears to be refcounting, it doesn't seem like the FIXME to limit its size is needed; it will only ever be as large as the number of HarfbuzzFaces needed

> Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.cpp:257
> +    if (result.get()->second.second) {

Shouldn't this test == 0?

[Kenichi Ishibashi]

Hi Evan,

Thank you for your prompt response.

(In reply to comment #2)
> - is the memory use of HarfbuzzFace a problem you've measured?

Not me, but other guys measured a large memory consumption on HarfbuzzFace.

> - why aren't higher-level caches in WebKit over fonts caching this as well?

Actually, WebKit has a font caching mechanism (Source/WebCore/platform/graphics/FontCache.{h,cpp}. 
The problem is the current implementation could call HB_NewFace() for the same uniqueID because HarfbuzzFace belongs to FontPlatformData and instances of FontPlatformData could have the same uniqueID of Skia font face.

> - since the cache appears to be refcounting, it doesn't seem like the FIXME to limit its size is needed; it will only ever be as large as the number of HarfbuzzFaces needed

Certainly.  I'll remove the comment.

> > Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.cpp:257
> > +    if (result.get()->second.second) {
> 
> Shouldn't this test == 0?

check-webkit-style complained: "Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]".

[Satish Sampath]

I have measured memory usage of this code. One of my test sites was kapook.com and I saw 10 copies of the same font being created, each one taking 4M of memory. This patch would be a welcome addition to reduce such memory usage.

[Evan Martin]

(In reply to comment #3)
> > - why aren't higher-level caches in WebKit over fonts caching this as well?
> 
> Actually, WebKit has a font caching mechanism (Source/WebCore/platform/graphics/FontCache.{h,cpp}. 
> The problem is the current implementation could call HB_NewFace() for the same uniqueID because HarfbuzzFace belongs to FontPlatformData and instances of FontPlatformData could have the same uniqueID of Skia font face.

Is it a bug that we can have more than one FontPlatformData for the same uniqueID?  (Another way of phrasing my question: why isn't this a problem on Windows?)

> > > Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.cpp:257
> > > +    if (result.get()->second.second) {
> > 
> > Shouldn't this test == 0?
> 
> check-webkit-style complained: "Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]".

Well, then, at least something like:
  if (!result.get()->second.second) {
?

[Evan Martin]

(In reply to comment #4)
> I have measured memory usage of this code. One of my test sites was kapook.com and I saw 10 copies of the same font being created, each one taking 4M of memory. This patch would be a welcome addition to reduce such memory usage.

Don't get me wrong -- I understand what the patch is accomplishing; I just don't want to be quick to jump to the wrong fix.  WebKit has many layers of caches like these and each one makes the app less predictable and consumes memory on top of the others.  (It is likely that bashi's patch is correct, I just want to be sure.)

[Kenichi Ishibashi]

Hi Evan,

Your concerns are definitely reasonable.

(In reply to comment #5)
> Is it a bug that we can have more than one FontPlatformData for the same uniqueID?  (Another way of phrasing my question: why isn't this a problem on Windows?)

It's not a bug.  Multiple FontPlatformData could have the same (Skia's) uniqueID.  For instance, the uniqueID could be the same for fonts which have the same font-family and different font-size.

I'm not sure there is the same problem on Windows since I don't know well enough Windows port.

> Well, then, at least something like:
>   if (!result.get()->second.second) {
> ?

Oh, how stupid I am. Will fix.

[Satish Sampath]

(In reply to comment #5)
> (In reply to comment #3)
> > > - why aren't higher-level caches in WebKit over fonts caching this as well?
> > 
> > Actually, WebKit has a font caching mechanism (Source/WebCore/platform/graphics/FontCache.{h,cpp}. 
> > The problem is the current implementation could call HB_NewFace() for the same uniqueID because HarfbuzzFace belongs to FontPlatformData and instances of FontPlatformData could have the same uniqueID of Skia font face.
> 
> Is it a bug that we can have more than one FontPlatformData for the same uniqueID?  (Another way of phrasing my question: why isn't this a problem on Windows?)

When I tested on Linux I saw HB_NewFace was called multiple times for the same font there as well. My guess is that this would be the same for windows. The difference in memory usage was because the font file used was different hence memory usage was lesser.

[Kenichi Ishibashi]

Created attachment 102939 [details]
Patch V1

[Evan Martin]

LGTM (note: I am not a WebKit reviewer)

I'm still a little confused as to why this is necessary.  I worry that there is a cache at a different layer that we are not properly using.

It looks like on Windows we use an HFONT in a similar place, but an HFONT is for a specific size, so I guess it makes sense that they don't need a cache there.  I wonder if HFONT uses a shared cache of the underlying font files inside Windows somewhere.  I think I've convinced myself this is ok.  :)

[Tony Chang]

Comment on attachment 102939 [details]
Patch V1

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

> Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.cpp:231
> +typedef HashMap<unsigned int, FaceCacheEntry, WTF::IntHash<unsigned int>, WTF::UnsignedWithZeroKeyHashTraits<unsigned int> > HarfbuzzFaceCache;

Nit: If you use |unsigned| instead of |unsigned int|, can you drop the last 2 template parameters?

[Tony Chang]

(In reply to comment #11)
> (From update of attachment 102939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102939&action=review
> 
> > Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.cpp:231
> > +typedef HashMap<unsigned int, FaceCacheEntry, WTF::IntHash<unsigned int>, WTF::UnsignedWithZeroKeyHashTraits<unsigned int> > HarfbuzzFaceCache;
> 
> Nit: If you use |unsigned| instead of |unsigned int|, can you drop the last 2 template parameters?

Nevermind, it looks like you need to override the default hash trait for unsigned int.

[Kenichi Ishibashi]

Committed r92425: <http://trac.webkit.org/changeset/92425>