Bug 63888

Summary: Font cache eviction must be based on the number of cached fonts rather than that of inactive fonts
Product: WebKit Reporter: Yuzo Fujishima <yuzo>
Component: Page LoadingAssignee: Yuzo Fujishima <yuzo>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ggaren, mitz, msaboff
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Yuzo Fujishima 2011-07-03 21:50:12 PDT
Currently font cache eviction is done when the number of the inactive fonts exceeds the limit.

I believe the eviction's purpose is to limit the memory usage. Then (A) evicting font when the
number of cached fonts exceeds the limit makes more sense than (B) evicting font when the number
of inactive fonts exceeds the limit.
Comment 1 Yuzo Fujishima 2011-07-03 21:59:31 PDT
Created attachment 99594 [details]
Patch
Comment 2 mitz 2011-07-03 22:02:05 PDT
Wouldn’t this lead to thrashing when the number of active fonts is near or over the threshold?
Comment 3 Yuzo Fujishima 2011-07-03 22:20:42 PDT
Hi, thank you for the very prompt review.

If the number of the active fonts exceeds the threshold, a font is evicted immediately if it becomes inactive. It needs to be re-cached if used again. If you are calling this thrashing, yes, it can happen.

To avoid that, we should set the (soft) limit high enough.

Suppose we can afford 250 fonts on memory.

Patch 99594 proposes that we allow inactive fonts in cache as far as the total number of cached font
doesn't exceed 250.

The current behavior is that we allow inactive fonts as far as it doesn't exceed 50. We don't allow 51 inactive fonts even when we have 100 fonts cached and there is room for 150 more. On the other hand, we allow 50 even when we already have 500 fonts cached. This doesn't make much sense to me.

If we can allow 50 inactive when we have 500 cached, to avoid thrashing, we should just set
the soft limit to 500 to get the same effect.
Comment 4 Michael Saboff 2011-07-06 13:59:23 PDT
The existing code can create many inactive fonts.  When a font is needed only as a system fall back font it will be an inactive font and never an active font.  These system fallback fonts are used to provide glyphs missing for a code point in some active font. This would tend to support that the number of inactive fonts needed as system fallback fonts is proportional to the number of active fonts.  The performance accessing fonts when over the soft limit will likely degrade dramatically due to thrashing.

Have you tried running performance tests with > 250 active fonts?

Although we could go to a more sophisticated scheme to manage the number of active and inactive fonts, simpler is better.  Since we aren't going to purge active fonts much like we don't purge active resources in the webcore cache, I'd prefer that we measure and manage the objects that we do purge.  In this case, that is inactive fonts. I don't think it make sense to adjust an "activeFonts" soft limit as a way to control the number of inactive fonts. Therefore I'd recommend that we keep the current mechanism.
Comment 5 Yuzo Fujishima 2011-07-06 22:31:02 PDT
Michael,

Thank you for the review.

(In reply to comment #4)
> The existing code can create many inactive fonts.  When a font is needed only as a system fall back font it will be an inactive font and never an active font.  These system fallback fonts are used to provide glyphs missing for a code point in some active font. This would tend to support that the number of inactive fonts needed as system fallback fonts is proportional to the number of active fonts.  The performance accessing fonts when over the soft limit will likely degrade dramatically due to thrashing.
> 
> Have you tried running performance tests with > 250 active fonts?

I saw the same kind of performance degradation as I did for small (50) cMaxInactiveFontData case.

> 
> Although we could go to a more sophisticated scheme to manage the number of active and inactive fonts, simpler is better.  Since we aren't going to purge active fonts much like we don't purge active resources in the webcore cache, I'd prefer that we measure and manage the objects that we do purge.  In this case, that is inactive fonts. I don't think it make sense to adjust an "activeFonts" soft limit as a way to control the number of inactive fonts. Therefore I'd recommend that we keep the current mechanism.

Is there a good way to pick the right values for cMaxInactiveFontData and cTargetInactiveFontData?
If there is, the current approach would be fine with me.
Comment 6 Michael Saboff 2011-07-07 15:38:42 PDT
(In reply to comment #5)
...
> Is there a good way to pick the right values for cMaxInactiveFontData and cTargetInactiveFontData?
> If there is, the current approach would be fine with me.

The selection on these two values I think is somewhat subjective.  I think the biggest variable is choosing what test you want to execute with little or no font thrashing.  I outlined one process in https://bugs.webkit.org/show_bug.cgi?id=62349#c13 using such preferred test(s). This process does require adding code to instrument the number of active and inactive fonts.

We found that 50 / 30 were acceptable values for cMaxInactiveFontData and cTargetInactiveFontData with the page load / memory usage / performance tests that we use.  When I say acceptable, that did include some purging, but provided comparable performance to before the fall back font changes introduced in https://bugs.webkit.org/show_bug.cgi?id=61875.

Although I don't know the makeup of the tests you use to measure memory / font usage, I suspect they use a large number of languages at the same time.  In my mind, this would lead to a greater number of inactive font due to being system fallback fonts.
Comment 7 Yuzo Fujishima 2011-07-10 17:54:27 PDT
Michael,

Thank you for the comments.

I'd like to close this as WONTFIX and reopen this when I have a better argument.
Comment 8 Eric Seidel (no email) 2011-09-06 15:36:10 PDT
Comment on attachment 99594 [details]
Patch

Cleared review? from attachment 99594 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).