Bug 123643

Summary: createFontFaceValue() should be smarter about overgrown cache.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, gyuyoung.kim, kling, macpherson, menard
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch ggaren: review+

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]
Comment 2 Geoffrey Garen 2013-11-01 15:45:39 PDT
Comment on attachment 215770 [details]

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

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>