Bug 224159

Summary: Wasted vector capacity in MatchedDeclarationsCache::MatchResult
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: darin, jonlee, koivisto, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=224313
Attachments:
Description Flags
Patch none

Description Simon Fraser (smfr) 2021-04-03 12:48:12 PDT 
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.
Comment 1 Radar WebKit Bug Importer 2021-04-04 22:16:47 PDT 
<rdar://problem/76205835>
Comment 2 Simon Fraser (smfr) 2021-04-05 14:03:44 PDT 
Created attachment 425203 [details]
Patch
Comment 3 Darin Adler 2021-04-05 14:26:10 PDT 
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.
Comment 4 Simon Fraser (smfr) 2021-04-05 15:10:12 PDT 
That place is:
    m_entries.add(hash, Entry { matchResult, RenderStyle::clonePtr(style), RenderStyle::clonePtr(parentStyle) });

So we could have Entry construct compacted vectors.
Comment 5 Darin Adler 2021-04-05 15:13:04 PDT 
(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.
Comment 6 Darin Adler 2021-04-05 16:05:41 PDT 
It seems safe to do a shrinkToFit on a const object.
Comment 7 Antti Koivisto 2021-04-07 09:56:57 PDT 
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.
Comment 8 Simon Fraser (smfr) 2021-04-08 08:44:53 PDT 

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