From 4b3bb3fcc9a3e6a9248cd4592e4f1f61a6462cf8 Mon Sep 17 00:00:00 2001 From: rune Date: Tue, 9 Feb 2016 04:06:28 -0800 Subject: [PATCH] Only cache nth-indices when child count > 32. When matching :nth-* selectors, we sparsely cache the child index count into a hashmap for the parent element. Doing this regardlessly gave us a performance penalty for small number of children as where noticed in a performance degradation for [1]. The new approach is to not cache any indices until we match an :nth-* selector for which we walk more than 32 siblings. The number 32 were proposed in the bug report, and it turns out to be quite suitable given the experiments which were done comparing the implementation not using a cache at all, and the implementation where we cached regardlessly. We trigger caching for nth-of-type indices based on the sibling count as well, but not the sibling-of-type count as that would cause terrible performance if the elements of the same type were sparse compared to other siblings. Gives a > 40% performance improvement for [1]. [1] blink_perf.css:PseudoClassSelectors. BUG=483338 TEST=blink_perf.css:PseudoClassSelectors Review URL: https://codereview.chromium.org/1655993005 Cr-Commit-Position: refs/heads/master@{#374356} --- .../Source/core/css/SelectorChecker.cpp | 52 +---- .../WebKit/Source/core/dom/NthIndexCache.cpp | 205 +++++++++++++----- .../WebKit/Source/core/dom/NthIndexCache.h | 48 ++-- 3 files changed, 172 insertions(+), 133 deletions(-) diff --git a/third_party/WebKit/Source/core/css/SelectorChecker.cpp b/third_party/WebKit/Source/core/css/SelectorChecker.cpp index 22d4d5fee6a7b2..2738219f31eb43 100644 --- a/third_party/WebKit/Source/core/css/SelectorChecker.cpp +++ b/third_party/WebKit/Source/core/css/SelectorChecker.cpp @@ -180,50 +180,6 @@ static bool isLastOfType(Element& element, const QualifiedName& type) return !ElementTraversal::nextSibling(element, HasTagName(type)); } -static int nthChildIndex(Element& element) -{ - if (NthIndexCache* nthIndexCache = element.document().nthIndexCache()) - return nthIndexCache->nthChildIndex(element); - - int index = 1; - for (const Element* sibling = ElementTraversal::previousSibling(element); sibling; sibling = ElementTraversal::previousSibling(*sibling)) - index++; - - return index; -} - -static int nthOfTypeIndex(Element& element, const QualifiedName& type) -{ - if (NthIndexCache* nthIndexCache = element.document().nthIndexCache()) - return nthIndexCache->nthChildIndexOfType(element, type); - int index = 1; - for (const Element* sibling = ElementTraversal::previousSibling(element, HasTagName(type)); sibling; sibling = ElementTraversal::previousSibling(*sibling, HasTagName(type))) - ++index; - return index; -} - -static int nthLastChildIndex(Element& element) -{ - if (NthIndexCache* nthIndexCache = element.document().nthIndexCache()) - return nthIndexCache->nthLastChildIndex(element); - - int index = 1; - for (const Element* sibling = ElementTraversal::nextSibling(element); sibling; sibling = ElementTraversal::nextSibling(*sibling)) - ++index; - return index; -} - -static int nthLastOfTypeIndex(Element& element, const QualifiedName& type) -{ - if (NthIndexCache* nthIndexCache = element.document().nthIndexCache()) - return nthIndexCache->nthLastChildIndexOfType(element, type); - - int index = 1; - for (const Element* sibling = ElementTraversal::nextSibling(element, HasTagName(type)); sibling; sibling = ElementTraversal::nextSibling(*sibling, HasTagName(type))) - ++index; - return index; -} - // Recursive check of selectors and combinators // It can return 4 different values: // * SelectorMatches - the selector matches the element e @@ -770,14 +726,14 @@ bool SelectorChecker::checkPseudoClass(const SelectorCheckingContext& context, M if (ContainerNode* parent = element.parentElementOrDocumentFragment()) { if (m_mode == ResolvingStyle) parent->setChildrenAffectedByForwardPositionalRules(); - return selector.matchNth(nthChildIndex(element)); + return selector.matchNth(NthIndexCache::nthChildIndex(element)); } break; case CSSSelector::PseudoNthOfType: if (ContainerNode* parent = element.parentElementOrDocumentFragment()) { if (m_mode == ResolvingStyle) parent->setChildrenAffectedByForwardPositionalRules(); - return selector.matchNth(nthOfTypeIndex(element, element.tagQName())); + return selector.matchNth(NthIndexCache::nthOfTypeIndex(element)); } break; case CSSSelector::PseudoNthLastChild: @@ -786,7 +742,7 @@ bool SelectorChecker::checkPseudoClass(const SelectorCheckingContext& context, M parent->setChildrenAffectedByBackwardPositionalRules(); if (!parent->isFinishedParsingChildren()) return false; - return selector.matchNth(nthLastChildIndex(element)); + return selector.matchNth(NthIndexCache::nthLastChildIndex(element)); } break; case CSSSelector::PseudoNthLastOfType: @@ -795,7 +751,7 @@ bool SelectorChecker::checkPseudoClass(const SelectorCheckingContext& context, M parent->setChildrenAffectedByBackwardPositionalRules(); if (!parent->isFinishedParsingChildren()) return false; - return selector.matchNth(nthLastOfTypeIndex(element, element.tagQName())); + return selector.matchNth(NthIndexCache::nthLastOfTypeIndex(element)); } break; case CSSSelector::PseudoTarget: diff --git a/third_party/WebKit/Source/core/dom/NthIndexCache.cpp b/third_party/WebKit/Source/core/dom/NthIndexCache.cpp index a039019abecbb9..de8c37d0d3c90b 100644 --- a/third_party/WebKit/Source/core/dom/NthIndexCache.cpp +++ b/third_party/WebKit/Source/core/dom/NthIndexCache.cpp @@ -24,20 +24,146 @@ NthIndexCache::~NthIndexCache() m_document->setNthIndexCache(nullptr); } -NthIndexData& NthIndexCache::ensureNthIndexDataFor(Node& parent) +namespace { + +// Generating the cached nth-index counts when the number of children +// exceeds this count. This number is picked based on testing +// querySelectorAll for :nth-child(3n+2) and :nth-of-type(3n+2) on an +// increasing number of children. + +const unsigned kCachedSiblingCountLimit = 32; + +unsigned uncachedNthChildIndex(Element& element) +{ + int index = 1; + for (const Element* sibling = ElementTraversal::previousSibling(element); sibling; sibling = ElementTraversal::previousSibling(*sibling)) + index++; + + return index; +} + +unsigned uncachedNthLastChildIndex(Element& element) +{ + int index = 1; + for (const Element* sibling = ElementTraversal::nextSibling(element); sibling; sibling = ElementTraversal::nextSibling(*sibling)) + ++index; + return index; +} + +unsigned uncachedNthOfTypeIndex(Element& element, unsigned& siblingCount) +{ + int index = 1; + const QualifiedName& tag = element.tagQName(); + for (const Element* sibling = ElementTraversal::previousSibling(element); sibling; sibling = ElementTraversal::previousSibling(*sibling)) { + if (sibling->tagQName() == tag) + ++index; + ++siblingCount; + } + return index; +} + +unsigned uncachedNthLastOfTypeIndex(Element& element, unsigned& siblingCount) +{ + int index = 1; + const QualifiedName& tag = element.tagQName(); + for (const Element* sibling = ElementTraversal::nextSibling(element); sibling; sibling = ElementTraversal::nextSibling(*sibling)) { + if (sibling->tagQName() == tag) + ++index; + ++siblingCount; + } + return index; +} + +} // namespace + +unsigned NthIndexCache::nthChildIndex(Element& element) +{ + if (element.isPseudoElement()) + return 1; + ASSERT(element.parentNode()); + NthIndexCache* nthIndexCache = element.document().nthIndexCache(); + NthIndexData* nthIndexData = nullptr; + if (nthIndexCache && nthIndexCache->m_parentMap) + nthIndexData = nthIndexCache->m_parentMap->get(element.parentNode()); + if (nthIndexData) + return nthIndexData->nthIndex(element); + unsigned index = uncachedNthChildIndex(element); + if (nthIndexCache && index > kCachedSiblingCountLimit) + nthIndexCache->cacheNthIndexDataForParent(element); + return index; +} + +unsigned NthIndexCache::nthLastChildIndex(Element& element) +{ + if (element.isPseudoElement()) + return 1; + ASSERT(element.parentNode()); + NthIndexCache* nthIndexCache = element.document().nthIndexCache(); + NthIndexData* nthIndexData = nullptr; + if (nthIndexCache && nthIndexCache->m_parentMap) + nthIndexData = nthIndexCache->m_parentMap->get(element.parentNode()); + if (nthIndexData) + return nthIndexData->nthLastIndex(element); + unsigned index = uncachedNthLastChildIndex(element); + if (nthIndexCache && index > kCachedSiblingCountLimit) + nthIndexCache->cacheNthIndexDataForParent(element); + return index; +} + +NthIndexData* NthIndexCache::nthTypeIndexDataForParent(Element& element) const +{ + ASSERT(element.parentNode()); + if (!m_parentMapForType) + return nullptr; + if (const IndexByType* map = m_parentMapForType->get(element.parentNode())) + return map->get(element.tagName()); + return nullptr; +} + +unsigned NthIndexCache::nthOfTypeIndex(Element& element) +{ + if (element.isPseudoElement()) + return 1; + NthIndexCache* nthIndexCache = element.document().nthIndexCache(); + if (nthIndexCache) { + if (NthIndexData* nthIndexData = nthIndexCache->nthTypeIndexDataForParent(element)) + return nthIndexData->nthOfTypeIndex(element); + } + unsigned siblingCount = 0; + unsigned index = uncachedNthOfTypeIndex(element, siblingCount); + if (nthIndexCache && siblingCount > kCachedSiblingCountLimit) + nthIndexCache->cacheNthOfTypeIndexDataForParent(element); + return index; +} + +unsigned NthIndexCache::nthLastOfTypeIndex(Element& element) { + if (element.isPseudoElement()) + return 1; + NthIndexCache* nthIndexCache = element.document().nthIndexCache(); + if (nthIndexCache) { + if (NthIndexData* nthIndexData = nthIndexCache->nthTypeIndexDataForParent(element)) + return nthIndexData->nthLastOfTypeIndex(element); + } + unsigned siblingCount = 0; + unsigned index = uncachedNthLastOfTypeIndex(element, siblingCount); + if (nthIndexCache && siblingCount > kCachedSiblingCountLimit) + nthIndexCache->cacheNthOfTypeIndexDataForParent(element); + return index; +} + +void NthIndexCache::cacheNthIndexDataForParent(Element& element) +{ + ASSERT(element.parentNode()); if (!m_parentMap) m_parentMap = adoptPtrWillBeNoop(new ParentMap()); - ParentMap::AddResult addResult = m_parentMap->add(&parent, nullptr); - if (addResult.isNewEntry) - addResult.storedValue->value = adoptPtrWillBeNoop(new NthIndexData()); - - ASSERT(addResult.storedValue->value); - return *addResult.storedValue->value; + ParentMap::AddResult addResult = m_parentMap->add(element.parentNode(), nullptr); + ASSERT(addResult.isNewEntry); + addResult.storedValue->value = adoptPtrWillBeNoop(new NthIndexData(*element.parentNode())); } -NthIndexCache::IndexByType& NthIndexCache::ensureTypeIndexMap(Node& parent) +NthIndexCache::IndexByType& NthIndexCache::ensureTypeIndexMap(ContainerNode& parent) { if (!m_parentMapForType) m_parentMapForType = adoptPtrWillBeNoop(new ParentMapForType()); @@ -50,20 +176,17 @@ NthIndexCache::IndexByType& NthIndexCache::ensureTypeIndexMap(Node& parent) return *addResult.storedValue->value; } -NthIndexData& NthIndexCache::nthIndexDataWithTagName(Element& element) +void NthIndexCache::cacheNthOfTypeIndexDataForParent(Element& element) { + ASSERT(element.parentNode()); IndexByType::AddResult addResult = ensureTypeIndexMap(*element.parentNode()).add(element.tagName(), nullptr); - if (addResult.isNewEntry) - addResult.storedValue->value = adoptPtrWillBeNoop(new NthIndexData()); - return *addResult.storedValue->value; + ASSERT(addResult.isNewEntry); + addResult.storedValue->value = adoptPtrWillBeNoop(new NthIndexData(*element.parentNode(), element.tagQName())); } -unsigned NthIndexData::nthIndex(Element& element) +unsigned NthIndexData::nthIndex(Element& element) const { - if (element.isPseudoElement()) - return 1; - if (!m_count) - return cacheNthIndices(element); + ASSERT(!element.isPseudoElement()); unsigned index = 0; for (Element* sibling = &element; sibling; sibling = ElementTraversal::previousSibling(*sibling), index++) { @@ -74,14 +197,12 @@ unsigned NthIndexData::nthIndex(Element& element) return index; } -unsigned NthIndexData::nthIndexOfType(Element& element, const QualifiedName& type) +unsigned NthIndexData::nthOfTypeIndex(Element& element) const { - if (element.isPseudoElement()) - return 1; - if (!m_count) - return cacheNthIndicesOfType(element, type); + ASSERT(!element.isPseudoElement()); + unsigned index = 0; - for (Element* sibling = &element; sibling; sibling = ElementTraversal::previousSibling(*sibling, HasTagName(type)), index++) { + for (Element* sibling = &element; sibling; sibling = ElementTraversal::previousSibling(*sibling, HasTagName(element.tagQName())), index++) { auto it = m_elementIndexMap.find(sibling); if (it != m_elementIndexMap.end()) return it->value + index; @@ -89,27 +210,18 @@ unsigned NthIndexData::nthIndexOfType(Element& element, const QualifiedName& typ return index; } -unsigned NthIndexData::nthLastIndex(Element& element) +unsigned NthIndexData::nthLastIndex(Element& element) const { - if (element.isPseudoElement()) - return 1; - unsigned index = nthIndex(element); - return m_count - index + 1; + return m_count - nthIndex(element) + 1; } -unsigned NthIndexData::nthLastIndexOfType(Element& element, const QualifiedName& type) +unsigned NthIndexData::nthLastOfTypeIndex(Element& element) const { - if (element.isPseudoElement()) - return 1; - unsigned index = nthIndexOfType(element, type); - return m_count - index + 1; + return m_count - nthOfTypeIndex(element) + 1; } -unsigned NthIndexData::cacheNthIndices(Element& element) +NthIndexData::NthIndexData(ContainerNode& parent) { - ASSERT(!element.isPseudoElement()); - ASSERT(m_elementIndexMap.isEmpty()); - unsigned index = 0; // The frequency at which we cache the nth-index for a set of siblings. // A spread value of 3 means every third Element will have its nth-index cached. // Using a spread value > 1 is done to save memory. Looking up the nth-index will @@ -117,22 +229,16 @@ unsigned NthIndexData::cacheNthIndices(Element& element) // elements will be traversed. const unsigned spread = 3; unsigned count = 0; - for (Element* sibling = ElementTraversal::firstChild(*element.parentNode()); sibling; sibling = ElementTraversal::nextSibling(*sibling)) { + for (Element* sibling = ElementTraversal::firstChild(parent); sibling; sibling = ElementTraversal::nextSibling(*sibling)) { if (!(++count % spread)) m_elementIndexMap.add(sibling, count); - if (sibling == &element) - index = count; } - ASSERT(count && index); + ASSERT(count); m_count = count; - return index; } -unsigned NthIndexData::cacheNthIndicesOfType(Element& element, const QualifiedName& type) +NthIndexData::NthIndexData(ContainerNode& parent, const QualifiedName& type) { - ASSERT(!element.isPseudoElement()); - ASSERT(m_elementIndexMap.isEmpty()); - unsigned index = 0; // The frequency at which we cache the nth-index of type for a set of siblings. // A spread value of 3 means every third Element of its type will have its nth-index cached. // Using a spread value > 1 is done to save memory. Looking up the nth-index of its type will @@ -140,15 +246,12 @@ unsigned NthIndexData::cacheNthIndicesOfType(Element& element, const QualifiedNa // will be equal to find 'spread' elements in the sibling set. const unsigned spread = 3; unsigned count = 0; - for (Element* sibling = ElementTraversal::firstChild(*element.parentNode(), HasTagName(type)); sibling; sibling = ElementTraversal::nextSibling(*sibling, HasTagName(type))) { + for (Element* sibling = ElementTraversal::firstChild(parent, HasTagName(type)); sibling; sibling = ElementTraversal::nextSibling(*sibling, HasTagName(type))) { if (!(++count % spread)) m_elementIndexMap.add(sibling, count); - if (sibling == &element) - index = count; } - ASSERT(count && index); + ASSERT(count); m_count = count; - return index; } DEFINE_TRACE(NthIndexData) diff --git a/third_party/WebKit/Source/core/dom/NthIndexCache.h b/third_party/WebKit/Source/core/dom/NthIndexCache.h index 074994be8063a6..5ae0d46e250292 100644 --- a/third_party/WebKit/Source/core/dom/NthIndexCache.h +++ b/third_party/WebKit/Source/core/dom/NthIndexCache.h @@ -21,17 +21,15 @@ class CORE_EXPORT NthIndexData final : public NoBaseWillBeGarbageCollected, unsigned> m_elementIndexMap; unsigned m_count = 0; @@ -45,38 +43,20 @@ class CORE_EXPORT NthIndexCache final { explicit NthIndexCache(Document&); ~NthIndexCache(); - unsigned nthChildIndex(Element& element) - { - ASSERT(element.parentNode()); - return ensureNthIndexDataFor(*element.parentNode()).nthIndex(element); - } - - unsigned nthChildIndexOfType(Element& element, const QualifiedName& type) - { - ASSERT(element.parentNode()); - return nthIndexDataWithTagName(element).nthIndexOfType(element, type); - } - - unsigned nthLastChildIndex(Element& element) - { - ASSERT(element.parentNode()); - return ensureNthIndexDataFor(*element.parentNode()).nthLastIndex(element); - } - - unsigned nthLastChildIndexOfType(Element& element, const QualifiedName& type) - { - ASSERT(element.parentNode()); - return nthIndexDataWithTagName(element).nthLastIndexOfType(element, type); - } + static unsigned nthChildIndex(Element&); + static unsigned nthLastChildIndex(Element&); + static unsigned nthOfTypeIndex(Element&); + static unsigned nthLastOfTypeIndex(Element&); private: using IndexByType = WillBeHeapHashMap>; using ParentMap = WillBeHeapHashMap, OwnPtrWillBeMember>; using ParentMapForType = WillBeHeapHashMap, OwnPtrWillBeMember>; - NthIndexData& ensureNthIndexDataFor(Node&); - IndexByType& ensureTypeIndexMap(Node&); - NthIndexData& nthIndexDataWithTagName(Element&); + void cacheNthIndexDataForParent(Element&); + void cacheNthOfTypeIndexDataForParent(Element&); + IndexByType& ensureTypeIndexMap(ContainerNode&); + NthIndexData* nthTypeIndexDataForParent(Element&) const; RawPtrWillBeMember m_document; OwnPtrWillBeMember m_parentMap;