Bug 17203

Summary:

REGRESSION: High CPU usage loading HTML5 spec

Product:

WebKit

Reporter:

Matt Lilek <dev+webkit>

Component:

New Bugs

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, hyatt, sam, webkit

Priority:

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+

Matt Lilek

Reported 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]

Attachments
Full sample (905.63 KB, text/plain) 2008-02-06 22:23 PST, Matt Lilek	no flags	Details
Patch that improves the performance of selectors. (63.84 KB, patch) 2008-02-07 16:12 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Patch to improve selector performance (63.85 KB, patch) 2008-02-07 16:14 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Ready for primetime. (64.28 KB, patch) 2008-02-07 17:29 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Patch with ChangeLog (73.44 KB, patch) 2008-02-08 11:52 PST, Dave Hyatt	eric: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Matt Lilek

Comment 1 2008-02-06 22:23:43 PST

Created attachment 18979 [details] Full sample

Robert Blaut

Comment 2 2008-02-07 05:20:40 PST

Confirmed.

Alexey Proskuryakov

Comment 3 2008-02-07 11:20:19 PST

Is it a regression from shipping Safari/WebKit? It should be P1 if it is.

Dave Hyatt

Comment 4 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.

Dave Hyatt

Comment 5 2008-02-07 16:12:28 PST

Created attachment 18993 [details] Patch that improves the performance of selectors.

Dave Hyatt

Comment 6 2008-02-07 16:14:25 PST

Created attachment 18994 [details] Patch to improve selector performance

Dave Hyatt

Comment 7 2008-02-07 17:03:44 PST

Comment on attachment 18994 [details] Patch to improve selector performance This needs work.

Dave Hyatt

Comment 8 2008-02-07 17:29:52 PST

Created attachment 18995 [details] Ready for primetime.

Mark Rowe (bdash)

Comment 9 2008-02-07 17:44:21 PST

<rdar://problem/5731323>

Dave Hyatt

Comment 10 2008-02-08 11:52:22 PST

Created attachment 19005 [details] Patch with ChangeLog

Eric Seidel (no email)

Comment 11 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.

Dave Hyatt

Comment 12 2008-02-09 14:49:35 PST

Fixed in r30112

Dave Hyatt

Comment 13 2008-02-09 14:52:49 PST

Followup in r30113 to back out something I checked in by accident.

Dave Hyatt

Comment 14 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! :(

Dave Hyatt

Comment 15 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.

David Kilzer (:ddkilzer)

Comment 16 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? :)

Dave Hyatt

Comment 17 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.

Note You need to log in before you can comment on or make changes to this bug.