Bug 17203

Summary: REGRESSION: High CPU usage loading HTML5 spec
Product: WebKit Reporter: Matt Lilek <dev+webkit>
Component: New BugsAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, hyatt, sam, webkit
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://www.whatwg.org/specs/web-apps/current-work/
Attachments:
Description Flags
Full sample
none
Patch that improves the performance of selectors.
none
Patch to improve selector performance
none
Ready for primetime.
none
Patch with ChangeLog eric: review+

Description Matt Lilek 2008-02-06 22:23:17 PST 
This is likely a result of the combination of the use of some fancy new CSS selectors and a large document, but loading the WHATWG's spec page results in significant CPU usage while loading.  r30053 nightly pegs my CPU at 100% for 10-15 seconds while its loading, while an older nightly (r29807) only spikes the CPU for 1-2 seconds (2 GHz Core 2 Duo, 2 GB RAM, 10.5.1).  Not sure what can be done about it, but it's probably worth making it known regardless.

Part of the sample results from my r30060 debug build:
Total number in stack (recursive counted multiple, when >=5):
        55       WTF::fastFree(void*)
        48       WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::~HashTableIterator()
        46       WebCore::AtomicString::impl() const
        45       WTF::fastMalloc(unsigned long)
        45       free
        40       WebCore::CSSStyleSelector::checkSelector(WebCore::CSSSelector*, WebCore::Element*, bool, bool)
        39       WebCore::CSSStyleSelector::checkOneSelector(WebCore::CSSSelector*, WebCore::Element*, bool, bool)
        39       WebCore::String::impl() const
        37       malloc
        36       WTF::HashMap<int, WebCore::GlyphPageTreeNode*, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<WebCore::GlyphPageTreeNode*> >::HashMap(WTF::HashMap<int, WebCore::GlyphPageTreeNode*, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<WebCore::GlyphPageTreeNode*> > const&)
        36       WTF::HashTable<int, std::pair<int, int>, WTF::PairFirstExtractor<std::pair<int, int> >, WTF::IntHash<unsigned int>, WTF::PairHashTraits<WTF::HashTraits<int>, WTF::HashTraits<int> >, WTF::HashTraits<int> >::HashTable(WTF::HashTable<int, std::pair<int, int>, WTF::PairFirstExtractor<std::pair<int, int> >, WTF::IntHash<unsigned int>, WTF::PairHashTraits<WTF::HashTraits<int>, WTF::HashTraits<int> >, WTF::HashTraits<int> > const&)
        36       malloc_zone_malloc
        34       WTF::HashTableConstIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::~HashTableConstIterator()
        34       WTF::isForbidden()
        32       WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::HashTableIterator(WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const&)
        32       szone_free
        31       WebCore::operator==(WebCore::AtomicString const&, WebCore::AtomicString const&)
        31       szone_malloc
        30       WTF::HashTableConstIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::HashTableConstIterator(WTF::HashTableConstIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const&)
        30       WebCore::CSSStyleSelector::matchRules(WebCore::CSSRuleSet*, int&, int&)
        28       WTF::RefPtr<WebCore::StringImpl>::get() const
        27       WebCore::CSSStyleSelector::matchRulesForList(WebCore::CSSRuleDataList*, int&, int&)
        26       WebCore::Font::~Font()
        26       WebCore::String::~String()
        25       WebCore::CSSStyleSelector::checkSelector(WebCore::CSSSelector*)
        25       WebCore::FontDescription::FontDescription(WebCore::FontDescription const&)
        24       WebCore::RenderStyle::~RenderStyle()
        24       __spin_lock
        23       WTF::RefPtr<WebCore::StringImpl>::~RefPtr()
        23       tiny_malloc_from_free_list
        22       WebCore::Font::Font(WebCore::Font const&)
        22       WebCore::StyleInheritedData::StyleInheritedData(WebCore::StyleInheritedData const&)
        21       WebCore::FontDescription::~FontDescription()
        20       WTF::HashMap<int, WebCore::GlyphPageTreeNode*, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<WebCore::GlyphPageTreeNode*> >::~HashMap()
        20       WTF::HashTable<int, std::pair<int, int>, WTF::PairFirstExtractor<std::pair<int, int> >, WTF::IntHash<unsigned int>, WTF::PairHashTraits<WTF::HashTraits<int>, WTF::HashTraits<int> >, WTF::HashTraits<int> >::~HashTable()
        20       WebCore::DataRef<WebCore::StyleInheritedData>::~DataRef()
        20       WebCore::FontFamily::FontFamily(WebCore::FontFamily const&)
        20       WebCore::String::String(char const*)
        20       std::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool>::pair(WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const&, bool const&)
        20       std::pair<WTF::HashTableIteratorAdapter<WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, WebCore::AtomicStringImpl*>, bool>::~pair()
        19       WTF::HashSet<WebCore::AtomicStringImpl*, WTF::PtrHash<WebCore::AtomicStringImpl*>, WTF::HashTraits<WebCore::AtomicStringImpl*> >::add(WebCore::AtomicStringImpl* const&)
        19       WebCore::CSSSelector::hasTag() const
        19       WebCore::FontFamily::~FontFamily()
        19       WebCore::QualifiedName::operator!=(WebCore::QualifiedName const&) const
        19       std::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool> WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::add<WebCore::AtomicStringImpl*, WebCore::AtomicStringImpl*, WTF::HashSetTranslator<true, WebCore::AtomicStringImpl*, WTF::HashTraits<WebCore::AtomicStringImpl*>, WTF::HashTraits<int>, WTF::PtrHash<WebCore::AtomicStringImpl*> > >(WebCore::AtomicStringImpl* const&, WebCore::AtomicStringImpl* const&)
        19       std::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool>::~pair()
        18       WTF::HashTableIteratorAdapter<WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, WebCore::AtomicStringImpl*>::~HashTableIteratorAdapter()
        18       WebCore::StyleInheritedData::~StyleInheritedData()
        17       std::pair<WTF::HashTableIteratorAdapter<WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, WebCore::AtomicStringImpl*>, bool>::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool>(std::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool> const&)
        16       WebCore::operator!=(WebCore::AtomicString const&, WebCore::AtomicString const&)
        15       WTF::HashTableConstIterator<int, std::pair<int, int>, WTF::PairFirstExtractor<std::pair<int, int> >, WTF::IntHash<unsigned int>, WTF::PairHashTraits<WTF::HashTraits<int>, WTF::HashTraits<int> >, WTF::HashTraits<int> >::~HashTableConstIterator()
        15       WebCore::CSSSelector::hasAttribute() const
        15       WebCore::CSSStyleSelector::applyProperty(int, WebCore::CSSValue*)
        15       WebCore::QualifiedName::operator==(WebCore::QualifiedName const&) const
        15       WebCore::StringImpl::StringImpl(char const*, unsigned int)
[snip]
Comment 1 Matt Lilek 2008-02-06 22:23:43 PST 
Created attachment 18979 [details]
Full sample
Comment 2 Robert Blaut 2008-02-07 05:20:40 PST 
Confirmed.
Comment 3 Alexey Proskuryakov 2008-02-07 11:20:19 PST 
Is it a regression from shipping Safari/WebKit? It should be P1 if it is.
Comment 4 Dave Hyatt 2008-02-07 11:52:14 PST 
This is :last-child.  I'll probably have to put in a parsing hack for it like KHTML did.
Comment 5 Dave Hyatt 2008-02-07 16:12:28 PST 
Created attachment 18993 [details]
Patch that improves the performance of selectors.
Comment 6 Dave Hyatt 2008-02-07 16:14:25 PST 
Created attachment 18994 [details]
Patch to improve selector performance
Comment 7 Dave Hyatt 2008-02-07 17:03:44 PST 
Comment on attachment 18994 [details]
Patch to improve selector performance

This needs work.
Comment 8 Dave Hyatt 2008-02-07 17:29:52 PST 
Created attachment 18995 [details]
Ready for primetime.
Comment 9 Mark Rowe (bdash) 2008-02-07 17:44:21 PST 
<rdar://problem/5731323>
Comment 10 Dave Hyatt 2008-02-08 11:52:22 PST 
Created attachment 19005 [details]
Patch with ChangeLog
Comment 11 Eric Seidel (no email) 2008-02-08 13:00:48 PST 
Comment on attachment 19005 [details]
Patch with ChangeLog

finishedParsingChildren() should probably be isFinishedParsingChildren() to make it more clear that it's just a bool lookup (and less likely to be confused with finishParsingChildren())

checkFirstChildRules and checkLastChildRules should be renamed now that they don't return a bool (otherwise it's confusing with checkEmptyRules)  I suggest markChildrenAffectedByLastChildRules() or similar.

We should eventually add some COMPILE_ASSERTs based on the size of the various classes we're trying to keep small.  (Filed bug 17217 about that just now).  Such would make it very easy when making these sorts of changes to know that you're not inflating core classes by accident.

I've never been clear on why the "(m_element == e)" checks are there.  Perhaps such could be moved into a more descriptive bool?
bool isMappedStyleRule = (m_element == e); // or whatever it's actually checking.

Otherwise looks good. r=me.
Comment 12 Dave Hyatt 2008-02-09 14:49:35 PST 
Fixed in r30112
Comment 13 Dave Hyatt 2008-02-09 14:52:49 PST 
Followup in r30113 to back out something I checked in by accident.
Comment 14 Dave Hyatt 2008-02-09 14:54:56 PST 
Another followup in r30114 since I missed the rendering/ directory.  So hard to sift through all of the changes in my tree! :(
Comment 15 Dave Hyatt 2008-02-09 14:57:29 PST 
The comedy continues.  I bungled the backout of the accidental change to CSSStyleSelector, since I forgot about the function rename from Eric's review.  r30115 has this change.
Comment 16 David Kilzer (:ddkilzer) 2008-02-20 13:37:56 PST 
(In reply to comment #15)
> The comedy continues.  I bungled the backout of the accidental change to
> CSSStyleSelector, since I forgot about the function rename from Eric's review. 
> r30115 has this change.

Have you tried git?  :)
Comment 17 Dave Hyatt 2008-02-20 13:42:13 PST 
(In reply to comment #16)
> (In reply to comment #15)
> > The comedy continues.  I bungled the backout of the accidental change to
> > CSSStyleSelector, since I forgot about the function rename from Eric's review. 
> > r30115 has this change.
> 
> Have you tried git?  :)
> 

That's way too logical and reasonable.