Bug 123643 - createFontFaceValue() should be smarter about overgrown cache.
Summary: createFontFaceValue() should be smarter about overgrown cache.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-01 15:40 PDT by Andreas Kling
Modified: 2013-11-01 18:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.58 KB, patch)
2013-11-01 15:42 PDT, Andreas Kling
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-11-01 15:40:45 PDT
Clearing the whole cache when it hits the size limit is silly.
Comment 1 Andreas Kling 2013-11-01 15:42:20 PDT
Created attachment 215770 [details]
Patch
Comment 2 Geoffrey Garen 2013-11-01 15:45:39 PDT
Comment on attachment 215770 [details]
Patch

Does this make something faster?
Comment 3 Darin Adler 2013-11-01 16:05:04 PDT
Comment on attachment 215770 [details]
Patch

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

> Source/WebCore/css/CSSValuePool.cpp:132
> -    // Just wipe out the cache and start rebuilding if it gets too big.
> +    // Remove one entry at random if the cache grows too large.
>      const int maximumFontFaceCacheSize = 128;
> -    if (m_fontFaceValueCache.size() > maximumFontFaceCacheSize)
> -        m_fontFaceValueCache.clear();
> +    if (m_fontFaceValueCache.size() + 1 >= maximumFontFaceCacheSize)

This check is kind of strange.

It lets the cache grow to a size of 127. After the add call below the cache might be one bigger or might not. If the cache already has 127, then it will remove one and then add one and get back to 127. So calling 128 the maximum is then silly.

I would consider either size >= maximum or size + 1 > maximum to be more sensible.
Comment 4 Andreas Kling 2013-11-01 18:51:42 PDT
Committed r158478: <http://trac.webkit.org/changeset/158478>