Bug 79195

Summary: Cache <font face> family lists in CSSValuePool.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 78070    
Description Flags
Semidecent patch koivisto: review+

Description Andreas Kling 2012-02-21 22:14:48 PST
Legacy content is typically quite heavy on <font face>. Parsing the face attribute to create a list of families is very expensive. We should cache the results of parsing a given attribute value to minimize time spent here.
Comment 1 Andreas Kling 2012-02-21 22:29:02 PST
Created attachment 128130 [details]
Semidecent patch
Comment 2 Antti Koivisto 2012-02-21 22:41:15 PST
Comment on attachment 128130 [details]
Semidecent patch

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
Comment 3 Andreas Kling 2012-02-21 23:33:36 PST
Committed r108451: <http://trac.webkit.org/changeset/108451>