[webkit-reviews] review granted: [Bug 79195] Cache <font face> family lists in CSSValuePool. : [Attachment 128130] Semidecent patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 79195: Cache <font face> family lists in CSSValuePool.
https://bugs.webkit.org/show_bug.cgi?id=79195

Attachment 128130: Semidecent patch
https://bugs.webkit.org/attachment.cgi?id=128130&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=128130&action=review


r=me with a mechanism to prevent unlimited pool growth in weird cases.

> Source/WebCore/ChangeLog:10
> +	   HTMLFontElements with "face" attributes are very common in legacy
web content.
> +	   Add a String->CSSValue cache for these in CSSValuePool and use it to
avoid
> +	   reparsing and recreating duplicate font face values.

A word about performance results might be good.

> Source/WebCore/css/CSSValuePool.cpp:140
> +PassRefPtr<CSSValueList> CSSValuePool::createFontFaceValue(const
AtomicString& string, CSSStyleSheet* contextStyleSheet)
> +{
> +    RefPtr<CSSValueList>& value = m_fontFaceValueCache.add(string,
0).first->second;
> +    if (!value)
> +	   value = CSSParser::parseFontFaceValue(string, contextStyleSheet);
> +    return value;
> +}

There should be a mechanism to prevent boundless growth in case this gets hits
repeatedly with different values. Something similar to what color value pool
has should work fine.

> Source/WebCore/css/CSSValuePool.h:81
>      IntegerValueCache m_percentValueCache;
>      IntegerValueCache m_numberValueCache;
>  
> +    typedef HashMap<AtomicString, RefPtr<CSSValueList> > FontFaceValueCache;

> +    FontFaceValueCache m_fontFaceValueCache;

It would be nice to renames all these Caches to Pools at some point