Bug 92748 - Web Inspector: add CSSStyleSheet memory instrumentation
Summary: Web Inspector: add CSSStyleSheet memory instrumentation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks: 87262 92962 92966 93130
  Show dependency treegraph
 
Reported: 2012-07-31 06:58 PDT by Yury Semikhatsky
Modified: 2012-08-03 09:21 PDT (History)
18 users (show)

See Also:


Attachments
Patch (31.49 KB, patch)
2012-07-31 07:50 PDT, Yury Semikhatsky
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-07-31 06:58:19 PDT
It includes instrumenting a bunch of related classes including StyleRule, StylePropertySet etc.
Comment 1 Yury Semikhatsky 2012-07-31 07:50:47 PDT
Created attachment 155537 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2012-07-31 08:02:38 PDT
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.
Comment 3 Yury Semikhatsky 2012-07-31 08:13:32 PDT
(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.
Comment 4 Ilya Tikhonovsky 2012-08-01 00:50:31 PDT
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
Comment 5 Antti Koivisto 2012-08-01 01:50:00 PDT
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?
Comment 6 Yury Semikhatsky 2012-08-01 06:25:57 PDT
(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.
Comment 7 Yury Semikhatsky 2012-08-01 06:27:00 PDT
(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.
Comment 8 Yury Semikhatsky 2012-08-01 06:30:26 PDT
Committed r124330: <http://trac.webkit.org/changeset/124330>