Bug 147446

Summary: Minimize children vector capacity changes in PlatformCALayerRemote::recursiveBuildTransaction()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Layout and RenderingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, simon.fraser, thorton, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2015-07-30 10:21:41 PDT
PlatformCALayerRemote::recursiveBuildTransaction() currently does:
1. Clear the children vector (which frees the underlying memory)
2. Append layer IDs to the vector one by one, causing the Vector capacity to grow

Since we know how many layer IDs we are going to add, we can minimize the number of Vector capacity changes by:
1. Resizing the children vector to the target size (which will grow or shrink the capacity as needed)
2. Update the layer IDs in the children vector, which won't cause any capacity change
Comment 1 Chris Dumez 2015-07-30 10:23:59 PDT
Created attachment 257839 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-07-30 10:44:17 PDT
Comment on attachment 257839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257839&action=review

Could we also reserveInitialCapacity()/uncheckedAppend()?

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:162
> +            for (size_t i = 0; i < m_children.size(); ++i)

Why change this?
Comment 3 Chris Dumez 2015-07-30 11:10:45 PDT
Comment on attachment 257839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257839&action=review

>> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:162
>> +            for (size_t i = 0; i < m_children.size(); ++i)
> 
> Why change this?

reserveInitialCapacity() would not be OK here as the m_properties.children is not a newly created Vector here (may have elements already). I guess we could do something like:
m_properties.children.shrink(0); // clear items without freeing underlying capacity
m_properties.children.reserve(reserveCapacity); // Resize underlying capacity
for (const auto& layer : m_children)
  m_properties.children.uncheckedAppend(layer->layerID());

That said, I still prefer my approach as I find it clearer than shrink() + reserveCapacity(). Also, the pattern I used is not unusual (see for e.g. copy*ToVector() in HashMap.h.
Comment 4 WebKit Commit Bot 2015-07-30 11:37:06 PDT
Comment on attachment 257839 [details]
Patch

Clearing flags on attachment: 257839

Committed r187594: <http://trac.webkit.org/changeset/187594>
Comment 5 WebKit Commit Bot 2015-07-30 11:37:11 PDT
All reviewed patches have been landed.  Closing bug.