Bug 186709 – SimpleLineLayout::FlowContents wastes 54KB of Vector capacity on nytimes.com

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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

[Radar WebKit Bug Importer]

<rdar://problem/41173793>

[alan]

(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?

[Simon Fraser (smfr)]

No, but you could easily add this to the code added in bug 186698.

[alan]

Created attachment 344335 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 344335 [details]
Patch

Clearing flags on attachment: 344335

Committed r233530: <https://trac.webkit.org/changeset/233530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Yusuke Suzuki]

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[]>).

[Saam Barati]

Seems like this may be a Speedometer2 regression on iOS

[Saam Barati]

(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.

[Saam Barati]

(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

[alan]

Committed r233728: <https://trac.webkit.org/changeset/233728>