Summary: | SimpleLineLayout::FlowContents wastes 54KB of Vector capacity on nytimes.com | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bfulgham, commit-queue, darin, saam, simon.fraser, webkit-bug-importer, ysuzuki, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2018-06-15 15:11:00 PDT
(In reply to Simon Fraser (smfr) from comment #0) > Tooling from bug 186698: > > Wasted capacity: 54096 bytes (used 7728 of 61824 bytes, utilization: 12.50%) > - 161 allocations > 1 0x108927092 > WTF::VectorBuffer<WebCore::SimpleLineLayout::FlowContents::Segment, > 8ul>::VectorBuffer(unsigned long, unsigned long) > 2 0x108926fa8 > WTF::Vector<WebCore::SimpleLineLayout::FlowContents::Segment, 8ul, > WTF::CrashOnOverflow, 16ul>::Vector<0ul, WTF::CrashOnOverflow, > 16ul>(WTF::Vector<WebCore::SimpleLineLayout::FlowContents::Segment, 0ul, > WTF::CrashOnOverflow, 16ul> const&) > 3 0x1088faaed > WTF::Vector<WebCore::SimpleLineLayout::FlowContents::Segment, 8ul, > WTF::CrashOnOverflow, 16ul>::Vector<0ul, WTF::CrashOnOverflow, > 16ul>(WTF::Vector<WebCore::SimpleLineLayout::FlowContents::Segment, 0ul, > WTF::CrashOnOverflow, 16ul> const&) > 4 0x1088fa790 > WebCore::SimpleLineLayout::FlowContents::FlowContents(WebCore:: > RenderBlockFlow const&) > 5 0x1088fab3d > WebCore::SimpleLineLayout::FlowContents::FlowContents(WebCore:: > RenderBlockFlow const&) > 6 0x10892f8a6 > WebCore::SimpleLineLayout::RunResolver::RunResolver(WebCore::RenderBlockFlow > const&, WebCore::SimpleLineLayout::Layout const&) > 7 0x10892f9c5 > WebCore::SimpleLineLayout::RunResolver::RunResolver(WebCore::RenderBlockFlow > const&, WebCore::SimpleLineLayout::Layout const&) > 8 0x1088f8309 WebCore::SimpleLineLayout::Layout::runResolver() const In initializeSegments(), we call segments.reserveCapacity(numberOfChildren); on the vector -and we recreate it every time the content changes. Is there stacktraces for the wasted allocations? No, but you could easily add this to the code added in bug 186698. Created attachment 344335 [details]
Patch
Comment on attachment 344335 [details] Patch Clearing flags on attachment: 344335 Committed r233530: <https://trac.webkit.org/changeset/233530> All reviewed patches have been landed. Closing bug. Comment on attachment 344335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344335&action=review > Source/WebCore/ChangeLog:9 > + The size of the m_segments vector in SimpleLineLayoutFlowContents is alway pre-computed and don't change after the initial append. Given that’s true then we could make an even-more-memory efficient implementation by storing a size alongside a std::unique_ptr<Segment[]>. Could even make a class called something like WTF::FixedVector to make that available. Not sure how often we’d use such a thing or how well it would interoperate with things that know how to work with WTF::vector. Comment on attachment 344335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344335&action=review >> Source/WebCore/ChangeLog:9 >> + The size of the m_segments vector in SimpleLineLayoutFlowContents is alway pre-computed and don't change after the initial append. > > Given that’s true then we could make an even-more-memory efficient implementation by storing a size alongside a std::unique_ptr<Segment[]>. Could even make a class called something like WTF::FixedVector to make that available. Not sure how often we’d use such a thing or how well it would interoperate with things that know how to work with WTF::vector. std::unique_ptr<T[]> typically allocates bytes for size since `delete [] rawPointer` requires this. So, if we want to have such FixedVector, we should allocate a memory with fastMalloc() internally (instead of having std::unique_ptr<T[]>). Seems like this may be a Speedometer2 regression on iOS (In reply to Saam Barati from comment #9) > Seems like this may be a Speedometer2 regression on iOS About ~3% on certain iOS devices, but not all. (In reply to Saam Barati from comment #10) > (In reply to Saam Barati from comment #9) > > Seems like this may be a Speedometer2 regression on iOS > > About ~3% on certain iOS devices, but not all. Interestingly, JS tests also regressed in this range. So perhaps it's a different change that caused this regression Committed r233728: <https://trac.webkit.org/changeset/233728> |