Bug 92748 – Web Inspector: add CSSStyleSheet memory instrumentation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

It includes instrumenting a bunch of related classes including StyleRule, StylePropertySet etc.

[Yury Semikhatsky]

Created attachment 155537 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 155537 [details]
Patch

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

> Source/WebCore/css/StyleSheetContents.cpp:445
> +void StyleSheetContents::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const

StyleSheetContents can be shared among several CSSStyleSheet's (and will be copied on write). I suggest talking to anttik to sort this out.

[Yury Semikhatsky]

(In reply to comment #2)
> (From update of attachment 155537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155537&action=review
> 
> > Source/WebCore/css/StyleSheetContents.cpp:445
> > +void StyleSheetContents::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
> 
> StyleSheetContents can be shared among several CSSStyleSheet's (and will be copied on write). I suggest talking to anttik to sort this out.

It's not a problem in case of memory instrumentation as we keep a set of all visited objects which prevents double counting.

[Ilya Tikhonovsky]

Comment on attachment 155537 [details]
Patch

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

lgtm

> Source/WebCore/css/CSSStyleSheet.cpp:177
> +    info.addInstrumentedMember(m_contents);
> +    info.addString(m_title);
> +    info.addInstrumentedMember(m_mediaQueries);

why did you skip other members?
    Node* m_ownerNode;
    CSSImportRule* m_ownerRule;

    mutable RefPtr<MediaList> m_mediaCSSOMWrapper;
    mutable Vector<RefPtr<CSSRule> > m_childRuleCSSOMWrappers;
    mutable OwnPtr<CSSRuleList> m_ruleListCSSOMWrapper;

> Source/WebCore/css/StyleSheetContents.cpp:450
> +

empty line

[Antti Koivisto]

Comment on attachment 155537 [details]
Patch

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

r=me

> Source/WebCore/css/StylePropertySet.cpp:1084
> +    MemoryClassInfo<StylePropertySet> info(memoryObjectInfo, this, MemoryInstrumentation::CSS);
> +    if (m_isMutable)
> +        info.addVectorPtr(m_mutablePropertyVector);
> +    else
> +        info.addRawBuffer(m_properties, m_arraySize * sizeof(CSSProperty));

CSSProperty contains a CSSValue. You are not taking its size (which may be significant) into account. Is this something you plan to do later?

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 155537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155537&action=review
> 
> lgtm
> 
> > Source/WebCore/css/CSSStyleSheet.cpp:177
> > +    info.addInstrumentedMember(m_contents);
> > +    info.addString(m_title);
> > +    info.addInstrumentedMember(m_mediaQueries);
> 
> why did you skip other members?
>     Node* m_ownerNode;
>     CSSImportRule* m_ownerRule;
> 
>     mutable RefPtr<MediaList> m_mediaCSSOMWrapper;
>     mutable Vector<RefPtr<CSSRule> > m_childRuleCSSOMWrappers;
>     mutable OwnPtr<CSSRuleList> m_ruleListCSSOMWrapper;
> 
They will be instrumented in the next patch.

> > Source/WebCore/css/StyleSheetContents.cpp:450
> > +
> 
> empty line
Fixed.

[Yury Semikhatsky]

(In reply to comment #5)
> (From update of attachment 155537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155537&action=review
> 
> r=me
> 
> > Source/WebCore/css/StylePropertySet.cpp:1084
> > +    MemoryClassInfo<StylePropertySet> info(memoryObjectInfo, this, MemoryInstrumentation::CSS);
> > +    if (m_isMutable)
> > +        info.addVectorPtr(m_mutablePropertyVector);
> > +    else
> > +        info.addRawBuffer(m_properties, m_arraySize * sizeof(CSSProperty));
> 
> CSSProperty contains a CSSValue. You are not taking its size (which may be significant) into account. Is this something you plan to do later?

I'm planning to instrument it and CSSRule in the next patch to keep the changes small enough for review.

[Yury Semikhatsky]

Committed r124330: <http://trac.webkit.org/changeset/124330>