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.
Created attachment 99594 [details] Patch
Wouldn’t this lead to thrashing when the number of active fonts is near or over the threshold?
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.
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.
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.
(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.
Michael, Thank you for the comments. I'd like to close this as WONTFIX and reopen this when I have a better argument.
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).