Bug 147446 - Minimize children vector capacity changes in PlatformCALayerRemote::recursiveBuildTransaction()
Summary: Minimize children vector capacity changes in PlatformCALayerRemote::recursive...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-30 10:21 PDT by Chris Dumez
Modified: 2015-07-30 11:37 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.18 KB, patch)
2015-07-30 10:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.