424622 - Tweak gfxFontCache hashing (and get rid of gfxPangoFontCache)

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Related to bug 401998 comment #8 -- the gfxFontCache is often a lot more precise in matching than the underlying platform font system, especially when it comes to sizes.  We should probably fix this, as we could improve cache hits -- and thus realize a Tp win.

Also, this would help us get rid of gfxPangoFontCache, which doesn't have an expiration mechanism but instead keeps 5000 pango fonts alive (when it hits 5000, it just blows away the entire cache (!)).

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Some quick numbers.. in one run of the talos pageset through a textrun-only testbed, without a whole lot of eviction (since the test completes in about a minute), at the end there are ~1800 entries in gfxPangoFontCache, and around ~2700 in the gfxFontCache.

Disabling gfxPangoFontCache thus causes us to use more memory, because we end up creating multiple pango fonts for what is essentially the same pango style.

Doing a quick test to just round the size in the style (ignoring font size adjust, for testing purposes), drops the number of entries down to 2170, so this is worth investigating.

Stuart is suggesting that beside the size, the langgroup and the sizeadjust are probably the remaining bits (because the sizeadjust could cause us to adjust to the same size as an already-cached pango font) that account for the difference.

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> Disabling gfxPangoFontCache thus causes us to use more memory, because we end
> up creating multiple pango fonts for what is essentially the same pango style.

PangoFonts get reused rather than duplicated if style and language match.

> Stuart is suggesting that beside the size, the langgroup and ...
> that account for the difference.

See bug 416725 comment 15 and comment 16 for discussion about the language and some data on removing the gfxPangoFontCache.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> (In reply to comment #1)
> > Disabling gfxPangoFontCache thus causes us to use more memory, because we end
> > up creating multiple pango fonts for what is essentially the same pango style.
> 
> PangoFonts get reused rather than duplicated if style and language match.

Get reused internal to Pango?  Or get reused with the PangoFontCache?  We were creating more pango fonts for sure without the cache..

> > Stuart is suggesting that beside the size, the langgroup and ...
> > that account for the difference.
> 
> See bug 416725 comment 15 and comment 16 for discussion about the language and
> some data on removing the gfxPangoFontCache.

Ah, yeah, the weight is the other thing that Stuart and I missed.  This isn't a linux-specific problem; XP and OSX would benefit for tightening up the caching.  Stuart said he'd look into it next week if he gets a chance.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> Get reused internal to Pango?  Or get reused with the PangoFontCache?

Internal to Pango.  Pango should be just giving back another reference to the same font if the closest match for the requested PangoFontDescription and language is the same.

> We were creating more pango fonts for sure without the cache..

I suspect that is because the gfxPangoFontCache (incorrectly) didn't include the language in the key to the hash table.

Do you mean more Pango fonts at once (due to language I think) or more sequentially?  Because the lifetime of the gfxFont in the gfxFontCache is quite short, what is essentially the same PangoFont (but with different addresses) would be created and destroyed more often.

> Ah, yeah, the weight is the other thing that Stuart and I missed.  This isn't
> a linux-specific problem; XP and OSX would benefit for tightening up the
> caching.  Stuart said he'd look into it next week if he gets a chance.

I don't expect that a loose gfxFontCache would be hurting Linux much in terms of memory, because a gfxPangoFont is really just a reference to PangoFont (which is shared by multiple gfxFonts) and a cairo font (which is also shared).

I think the issues are more wrt speed.  On Linux, the gfxFontCache is really a mapping from a desired family/style combination to an actual font, and finding the closest match is slow (FcFontSort through pango_context_load_font).

The other thing that can affect speed is that metric calculation is slow (bug 403618).  The metrics are cached in the PangoFont, but if the PangoFont gets destroyed and recreated then the metrics need to be recalculated.

The optimum expiration time (for a memory/speed tradeoff) on Linux may well be longer than other platforms (or possibly shorter because I don't know how expensive finding a match is on other platforms).

Comment 5

[Mike Schroepfer]

Spoke with vlad - we'd take a well tested fix but will not block

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

removes pango cache and rounds style.

still need to get accurate talos numbers.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 313368 [details] [diff] [review]
patch v.1

ran against talos standalone and saw only noise.

Comment 8

[Christopher Blizzard (:blizzard)]

Noise as in "it's better and there's no problem" or "made no difference at all?"

Comment 9

[Doug Turner (:dougt)]

the standalone page set is only 20 pages or so.  Because of the limited number of fonts in this small set, i can not see any gain nor any loss.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 313368 [details] [diff] [review]
patch v.1

Doug: are you aware that although Pango uses fixed point for size, it has a
scale factor of 1024?  So this patch would (or is trying to) change behavior.
I'm don't know whether that would be a bad change, but it's a little scary.

A (very) small amount of rounding could be done without changing the size of
the PangoFont created (but obviously wouldn't make as much difference to
performance - whatever that difference is):

style.size = ROUND(PANGO_SCALE * style.size) / PANGO_SCALE;

Also for the rounding to make any difference, the modified "style" needs to be
used in gfxFontCache::GetCache()->Lookup() and new gfxPangoFont() (rather than
aStyle).

I'd try increasing the gfxFontCache generation timeout to 90 seconds on Linux
to compensate for the removal of the gfxPangoFontCache.  Increasing beyond
that didn't help performance on my talos tests (with 1 run through ~400 pages).

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to comment #10)
> I'd try increasing the gfxFontCache generation timeout to 90 seconds on Linux

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/thebes/public/gfxFont.h&rev=1.103&mark=157#151

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 313368 [details] [diff] [review]
patch v.1

need to use a scaling factor.

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.1

includes karls comments.  not tested w/ talos

Comment 14

[Doug Turner (:dougt)]

stuart says he has a better approach.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is probably wanted 1.9.1, though I think that if we do the new freetype font backend for 1.9.1 this will just go away.

Comment 16

[Florian Bender]

Is this still relevant?

Comment 17

[Wayne Mery (:wsmwk)]

(In reply to Florian Bender from comment #16)
> Is this still relevant?

perhaps Jim knows

Comment 18

[Jim Mathies [:jimm]]

gfxPangoFontCache isn't around anymore, not sure about the rest of the font work. Since we haven't touched this since 2008, doubt we will.