Improve node statistics for rare data
Created attachment 350167 [details] Improved the logging
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?
(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.
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?
(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
Committed r236241: <https://trac.webkit.org/changeset/236241>
<rdar://problem/44630168>
Created attachment 350172 [details] Patch to enable the statistics every 100 nodes