Bug 224159 – Wasted vector capacity in MatchedDeclarationsCache::MatchResult

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: CSS

[Simon Fraser (smfr)]

With the patch from bug 186698 and loading nytimes article pages, we see wasted vector capacity under MatchResult:

Wasted capacity: 12144 bytes (used 992 of 13136 bytes, utilization: 7.55%) - 55 allocations
5   0x3ddd1abb2 WTF::VectorBuffer<WebCore::Style::MatchedProperties, 0ul, WTF::FastMalloc>::VectorBuffer(unsigned long, unsigned long)
6   0x3ddd1aa65 WTF::Vector<WebCore::Style::MatchedProperties, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::Vector(WTF::Vector<WebCore::Style::MatchedProperties, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
7   0x3ddd1aa0d WTF::Vector<WebCore::Style::MatchedProperties, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::Vector(WTF::Vector<WebCore::Style::MatchedProperties, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
8   0x3ddd1a9e0 WebCore::Style::MatchResult::MatchResult(WebCore::Style::MatchResult const&)
9   0x3ddd1519d WebCore::Style::MatchResult::MatchResult(WebCore::Style::MatchResult const&)
10  0x3ddd15056 WebCore::Style::MatchedDeclarationsCache::add(WebCore::RenderStyle const&, WebCore::RenderStyle const&, unsigned int, WebCore::Style::MatchResult const&)
11  0x3ddd71e8e WebCore::Style::Resolver::applyMatchedProperties(WebCore::Style::Resolver::State&, WebCore::Style::MatchResult const&, WebCore::Style::Resolver::UseMatchedDeclarationsCache)
12  0x3ddd709ab WebCore::Style::Resolver::styleForElement(WebCore::Element const&, WebCore::RenderStyle const*, WebCore::RenderStyle const*, WebCore::RuleMatchingBehavior, WebCore::SelectorFilter const*)
13  0x3dc2ca424 WebCore::Element::resolveStyle(WebCore::RenderStyle const*)
14  0x3dde53661 WebCore::SVGElement::resolveCustomStyle(WebCore::RenderStyle const&, WebCore::RenderStyle const*)
15  0x3ddd8c5a6 WebCore::Style::TreeResolver::styleForStyleable(WebCore::Styleable const&, WebCore::RenderStyle const&)
16  0x3ddd8cbca WebCore::Style::TreeResolver::resolveElement(WebCore::Element&)
17  0x3ddd8f46c WebCore::Style::TreeResolver::resolveComposedTree()
18  0x3ddd902d2 WebCore::Style::TreeResolver::resolve()
19  0x3dc1e6b07 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)
20  0x3dc1e7720 WebCore::Document::updateStyleIfNeeded()

Debugging suggests this is matchResult.authorDeclarations.

Nothing shrinks the vectors in the m_entries.add() Entry.

[Radar WebKit Bug Importer]

<rdar://problem/76205835>

[Simon Fraser (smfr)]

Created attachment 425203 [details]
Patch

[Darin Adler]

Comment on attachment 425203 [details]
Patch

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

> Source/WebCore/style/ElementRuleCollector.cpp:602
> +    m_result.userAgentDeclarations.shrinkToFit();
> +    m_result.userDeclarations.shrinkToFit();
> +    m_result.authorDeclarations.shrinkToFit();

Great to solve this: I don’t fully understand the choice of putting it here.

Presumably the reason we shrink these is that we are keeping these around for a while. Also, why don’t we need this when matchUARules is called directly?

It seems the place we want these calls are when these vectors go from being temporary things to being long-lived, and, to me, the end of this function is not obviously that place.

[Simon Fraser (smfr)]

That place is:
    m_entries.add(hash, Entry { matchResult, RenderStyle::clonePtr(style), RenderStyle::clonePtr(parentStyle) });

So we could have Entry construct compacted vectors.

[Darin Adler]

(In reply to Simon Fraser (smfr) from comment #4)
> That place is:
>     m_entries.add(hash, Entry { matchResult, RenderStyle::clonePtr(style),
> RenderStyle::clonePtr(parentStyle) });
> 
> So we could have Entry construct compacted vectors.

Yes, or make a ElementRuleCollector function that does allthe shrinkToFit calls and call it there.

[Darin Adler]

It seems safe to do a shrinkToFit on a const object.

[Antti Koivisto]

Maybe just add MatchResult::shrinkToFit and call it before creating the cache entry? In most cases MatchResults are temporaries so any work to shrink-to-fit is wasted.

[Simon Fraser (smfr)]

*** This bug has been marked as a duplicate of bug 224313 ***