Bug 186696

Summary: MediaQuerySet wastes a lot of vector capacity
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, koivisto, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Simon Fraser (smfr) 2018-06-15 14:43:33 PDT 
On theverge.com, we waste about half a meg of vector capacity here:

Wasted capacity: 589624 bytes (used 39368 of 628992 bytes, utilization: 6.26%) - 702 allocations
1   0x10743f765 WTF::VectorBuffer<WebCore::MediaQuery, 0ul>::VectorBuffer()
2   0x10743f745 WTF::Vector<WebCore::MediaQuery, 0ul, WTF::CrashOnOverflow, 16ul>::Vector()
3   0x1074284f5 WTF::Vector<WebCore::MediaQuery, 0ul, WTF::CrashOnOverflow, 16ul>::Vector()
4   0x1074284b6 WebCore::MediaQuerySet::MediaQuerySet()
5   0x107428515 WebCore::MediaQuerySet::MediaQuerySet()
6   0x1074283be WebCore::MediaQuerySet::create()
7   0x1075404be WebCore::MediaQueryParser::MediaQueryParser(WebCore::MediaQueryParser::ParserType, WebCore::MediaQueryParserContext)
8   0x10753f534 WebCore::MediaQueryParser::MediaQueryParser(WebCore::MediaQueryParser::ParserType, WebCore::MediaQueryParserContext)
Comment 1 Simon Fraser (smfr) 2018-06-15 14:50:50 PDT 
Need to re-do the changes from r204006
Comment 2 Radar WebKit Bug Importer 2018-06-15 14:51:23 PDT 
<rdar://problem/41172850>
Comment 3 Simon Fraser (smfr) 2018-06-15 15:00:28 PDT 
Tooling in bug 186698.
Comment 4 Chris Dumez 2018-06-15 16:26:42 PDT 
Created attachment 342856 [details]
Patch
Comment 5 Simon Fraser (smfr) 2018-06-15 16:29:44 PDT 
Comment on attachment 342856 [details]
Patch

Kling's previous changes here did a lot more shrinking: https://trac.webkit.org/changeset/204006. I also wonder if we should shrink MediaQuery's m_expressions vector.
Comment 6 Chris Dumez 2018-06-15 16:30:42 PDT 
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 342856 [details]
> Patch
> 
> Kling's previous changes here did a lot more shrinking:
> https://trac.webkit.org/changeset/204006. I also wonder if we should shrink
> MediaQuery's m_expressions vector.

The code has not changed and is still there:
void MediaQuerySet::shrinkToFit()
{
    m_queries.shrinkToFit();
    for (auto& query : m_queries)
        query.shrinkToFit();
}
Comment 7 Simon Fraser (smfr) 2018-06-15 16:32:44 PDT 
Comment on attachment 342856 [details]
Patch

OK
Comment 8 Chris Dumez 2018-06-15 16:36:40 PDT 
(In reply to Chris Dumez from comment #6)
> (In reply to Simon Fraser (smfr) from comment #5)
> > Comment on attachment 342856 [details]
> > Patch
> > 
> > Kling's previous changes here did a lot more shrinking:
> > https://trac.webkit.org/changeset/204006. I also wonder if we should shrink
> > MediaQuery's m_expressions vector.
> 
> The code has not changed and is still there:
> void MediaQuerySet::shrinkToFit()
> {
>     m_queries.shrinkToFit();
>     for (auto& query : m_queries)
>         query.shrinkToFit();
> }

I am looking at r204006 but I am not convinced it does more shrinking. We still shrink both the MediaQuerySet and every query in the vector.

Also, on ToT, MediaQuerySet::create() is only called in MediaQueryParser, and shrink there after we're done parsing. 

MediaQuerySet::create(const String&, MediaQueryParserContext&), calls MediaQueryParser::parseMediaQuerySet() which calls MediaQueryParser(MediaQuerySetParser, context).parseInternal(range) where I do the shrinking.

I am therefore confident I shrink all MediaQuerySet objects, right after they're parsed.
Comment 9 WebKit Commit Bot 2018-06-15 17:24:13 PDT 
Comment on attachment 342856 [details]
Patch

Clearing flags on attachment: 342856

Committed r232898: <https://trac.webkit.org/changeset/232898>
Comment 10 WebKit Commit Bot 2018-06-15 17:24:15 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Fraser (smfr) 2018-06-16 16:47:45 PDT 
*** Bug 186713 has been marked as a duplicate of this bug. ***