224159 2021-04-03 12:48:12 -0700 Wasted vector capacity in MatchedDeclarationsCache::MatchResult 2021-04-08 08:44:53 -0700 1 1 1 Unclassified WebKit CSS Safari Technology Preview Unspecified Unspecified RESOLVED DUPLICATE 224313 https://bugs.webkit.org/show_bug.cgi?id=224313 InRadar P2 Normal --- 1 simon.fraser simon.fraser darin jonlee koivisto simon.fraser webkit-bug-importer oldest_to_newest 1746813 0 simon.fraser 2021-04-03 12:48:12 -0700 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. 1746899 1 webkit-bug-importer 2021-04-04 22:16:47 -0700 <rdar://problem/76205835> 1747102 2 425203 simon.fraser 2021-04-05 14:03:44 -0700 Created attachment 425203 Patch 1747109 3 425203 darin 2021-04-05 14:26:10 -0700 Comment on attachment 425203 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. 1747127 4 simon.fraser 2021-04-05 15:10:12 -0700 That place is: m_entries.add(hash, Entry { matchResult, RenderStyle::clonePtr(style), RenderStyle::clonePtr(parentStyle) }); So we could have Entry construct compacted vectors. 1747128 5 darin 2021-04-05 15:13:04 -0700 (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. 1747149 6 darin 2021-04-05 16:05:41 -0700 It seems safe to do a shrinkToFit on a const object. 1747932 7 koivisto 2021-04-07 09:56:57 -0700 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. 1748305 8 simon.fraser 2021-04-08 08:44:53 -0700 *** This bug has been marked as a duplicate of bug 224313 *** 425203 2021-04-05 14:03:44 -0700 2021-04-05 15:46:53 -0700 Patch bug-224159-20210405140343.patch text/plain 1700 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMjc1NDQ5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjY0OGIxMGVmNGMyMTAz OWJlYjNlNDY3OWMxMDg4ODFiYTkwNTc0Ny4uMTg5OTVmYmQ2MzFjYTlmMGFjYjllYWUxN2RiNTNj OWY0NzQxNGI5MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIxLTA0LTA1ICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgorCisgICAgICAgIFdhc3RlZCB2ZWN0 b3IgY2FwYWNpdHkgaW4gTWF0Y2hlZERlY2xhcmF0aW9uc0NhY2hlOjpNYXRjaFJlc3VsdAorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI0MTU5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWF0Y2hSZXN1bHQu bV9yZXN1bHQuYXV0aG9yRGVjbGFyYXRpb25zIHdhcyBsZWZ0IHdpdGggZXhjZXNzIGNhcGFjaXR5 IHVuZGVyCisgICAgICAgIFNWR0VsZW1lbnQ6OnJlc29sdmVDdXN0b21TdHlsZSgpLCB3aGljaCB3 YXN0ZWQgdXAgdG8gMTJLQiBvbiBueXRpbWVzIGFydGljbGVzLgorCisgICAgICAgICogc3R5bGUv RWxlbWVudFJ1bGVDb2xsZWN0b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3R5bGU6OkVsZW1l bnRSdWxlQ29sbGVjdG9yOjptYXRjaEFsbFJ1bGVzKToKKwogMjAyMS0wNC0wMyAgU2ltb24gRnJh c2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KIAogICAgICAgICBXYXN0ZWQgdmVjdG9yIGNh cGFjaXR5IGluIEZFQ29sb3JNYXRyaXggYW5kIGZpbHRlcnMKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3N0eWxlL0VsZW1lbnRSdWxlQ29sbGVjdG9yLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3N0 eWxlL0VsZW1lbnRSdWxlQ29sbGVjdG9yLmNwcAppbmRleCBlMTgwY2NkZmE5ZjY4ZmFhYzA5MTBj MjMzYzIxOGQ3ZWZkNmEwNGE0Li4wZmJlZmM0NmUyYTgwNzNjYmI3MzA1ZGIzNzQzMzc4YzVhOTIw YmY5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9zdHlsZS9FbGVtZW50UnVsZUNvbGxlY3Rv ci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvc3R5bGUvRWxlbWVudFJ1bGVDb2xsZWN0b3IuY3Bw CkBAIC01OTYsNiArNTk2LDEwIEBAIHZvaWQgRWxlbWVudFJ1bGVDb2xsZWN0b3I6Om1hdGNoQWxs UnVsZXMoYm9vbCBtYXRjaEF1dGhvckFuZFVzZXJTdHlsZXMsIGJvb2wgaW5jCiAgICAgICAgIC8v IFJ1bGVzIGZyb20gdGhlIGhvc3Qgc2NvcGUgb3ZlcnJpZGUgaW5saW5lIHN0eWxlLgogICAgICAg ICB0cmFuc2Zlck1hdGNoZWRSdWxlcyhEZWNsYXJhdGlvbk9yaWdpbjo6QXV0aG9yLCBTY29wZU9y ZGluYWw6OkNvbnRhaW5pbmdIb3N0KTsKICAgICB9CisgICAgCisgICAgbV9yZXN1bHQudXNlckFn ZW50RGVjbGFyYXRpb25zLnNocmlua1RvRml0KCk7CisgICAgbV9yZXN1bHQudXNlckRlY2xhcmF0 aW9ucy5zaHJpbmtUb0ZpdCgpOworICAgIG1fcmVzdWx0LmF1dGhvckRlY2xhcmF0aW9ucy5zaHJp bmtUb0ZpdCgpOwogfQogCiB2b2lkIEVsZW1lbnRSdWxlQ29sbGVjdG9yOjphZGRFbGVtZW50SW5s aW5lU3R5bGVQcm9wZXJ0aWVzKGJvb2wgaW5jbHVkZVNNSUxQcm9wZXJ0aWVzKQo=