Bug 189775 – Improve node statistics for rare data

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

Improve node statistics for rare data

[Ryosuke Niwa]

Created attachment 350167 [details]
Improved the logging

[Simon Fraser (smfr)]

Comment on attachment 350167 [details]
Improved the logging

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

> Source/WebCore/dom/Node.cpp:130
> +static const char* stringForRareDataUseType(NodeRareData::UseType useType)
> +{
> +    switch (useType) {
> +    case NodeRareData::UseType::ConnectedFrameCount:
> +        return "ConnectedFrameCount";
> +    case NodeRareData::UseType::NodeList:
> +        return "NodeList";
> +    case NodeRareData::UseType::MutationObserver:
> +        return "MutationObserver";
> +    case NodeRareData::UseType::TabIndex:
> +        return "TabIndex";
> +    case NodeRareData::UseType::StyleFlags:
> +        return "StyleFlags";
> +    case NodeRareData::UseType::MinimumSize:
> +        return "MinimumSize";
> +    case NodeRareData::UseType::ScrollingPosition:
> +        return "ScrollingPosition";
> +    case NodeRareData::UseType::ComputedStyle:
> +        return "ComputedStyle";
> +    case NodeRareData::UseType::Dataset:
> +        return "Dataset";
> +    case NodeRareData::UseType::ClassList:
> +        return "ClassList";
> +    case NodeRareData::UseType::ShadowRoot:
> +        return "ShadowRoot";
> +    case NodeRareData::UseType::CustomElementQueue:
> +        return "CustomElementQueue";
> +    case NodeRareData::UseType::AttributeMap:
> +        return "AttributeMap";
> +    case NodeRareData::UseType::InteractionObserver:
> +        return "InteractionObserver";
> +    case NodeRareData::UseType::PseudoElements:
> +        return "PseudoElements";
> +    }
> +    return nullptr;
> +}

It would be nicer to implement TextStream& operator<<(TextStream& ts, NodeRareData::UseType) and do the outputting by building a TextStream. Then we can use that for log streams as well.

> Source/WebCore/dom/Node.cpp:175
> +            for (auto type : useTypes) {
> +                UNUSED_PARAM(type);
> +                useTypeCount++;
> +            }

Why not implement OptionSet::numberOfBitsSet() with one of the classic bit-counting algorithms?

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 350167 [details]
> Improved the logging
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350167&action=review
> 
> > Source/WebCore/dom/Node.cpp:130
> > +static const char* stringForRareDataUseType(NodeRareData::UseType useType)
> > +{
> > +    switch (useType) {
> > +    case NodeRareData::UseType::ConnectedFrameCount:
> > +        return "ConnectedFrameCount";
> > +    case NodeRareData::UseType::NodeList:
> > +        return "NodeList";
> > +    case NodeRareData::UseType::MutationObserver:
> > +        return "MutationObserver";
> > +    case NodeRareData::UseType::TabIndex:
> > +        return "TabIndex";
> > +    case NodeRareData::UseType::StyleFlags:
> > +        return "StyleFlags";
> > +    case NodeRareData::UseType::MinimumSize:
> > +        return "MinimumSize";
> > +    case NodeRareData::UseType::ScrollingPosition:
> > +        return "ScrollingPosition";
> > +    case NodeRareData::UseType::ComputedStyle:
> > +        return "ComputedStyle";
> > +    case NodeRareData::UseType::Dataset:
> > +        return "Dataset";
> > +    case NodeRareData::UseType::ClassList:
> > +        return "ClassList";
> > +    case NodeRareData::UseType::ShadowRoot:
> > +        return "ShadowRoot";
> > +    case NodeRareData::UseType::CustomElementQueue:
> > +        return "CustomElementQueue";
> > +    case NodeRareData::UseType::AttributeMap:
> > +        return "AttributeMap";
> > +    case NodeRareData::UseType::InteractionObserver:
> > +        return "InteractionObserver";
> > +    case NodeRareData::UseType::PseudoElements:
> > +        return "PseudoElements";
> > +    }
> > +    return nullptr;
> > +}
> 
> It would be nicer to implement TextStream& operator<<(TextStream& ts,
> NodeRareData::UseType) and do the outputting by building a TextStream. Then
> we can use that for log streams as well.

Yeah, I did consider doing that but that seemed like a lot of rewrites so we should probably do it in a separate patch from this.

> 
> > Source/WebCore/dom/Node.cpp:175
> > +            for (auto type : useTypes) {
> > +                UNUSED_PARAM(type);
> > +                useTypeCount++;
> > +            }
> 
> Why not implement OptionSet::numberOfBitsSet() with one of the classic
> bit-counting algorithms?

I think OptionSet::size() is being added in https://bugs.webkit.org/show_bug.cgi?id=189633. We can simply use that once that's available.

[Daniel Bates]

Comment on attachment 350167 [details]
Improved the logging

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

> Source/WebCore/dom/Element.cpp:3445
> +#if DUMP_NODE_STATISTICS

I am assuming DUMP_NODE_STATISTICS is always defined.

> Source/WebCore/dom/NodeRareData.h:252
> +#if defined(DUMP_NODE_STATISTICS) && DUMP_NODE_STATISTICS

Is DUMP_NODE_STATISTICS always defined? or not?

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #4)
> Comment on attachment 350167 [details]
> Improved the logging
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350167&action=review
> 
> > Source/WebCore/dom/Element.cpp:3445
> > +#if DUMP_NODE_STATISTICS

It's always defined in Node.h

> I am assuming DUMP_NODE_STATISTICS is always defined.
> 
> > Source/WebCore/dom/NodeRareData.h:252
> > +#if defined(DUMP_NODE_STATISTICS) && DUMP_NODE_STATISTICS
> 
> Is DUMP_NODE_STATISTICS always defined? or not?

NodeRareData.h can be included in places where Node.h isn't included. In those cases, we don't want to cause a compiler error or include Node.h

[Ryosuke Niwa]

Committed r236241: <https://trac.webkit.org/changeset/236241>

[Radar WebKit Bug Importer]

<rdar://problem/44630168>

[Ryosuke Niwa]

Created attachment 350172 [details]
Patch to enable the statistics every 100 nodes