|Summary:||Minimize children vector capacity changes in PlatformCALayerRemote::recursiveBuildTransaction()|
|Product:||WebKit||Reporter:||Chris Dumez <cdumez>|
|Component:||Layout and Rendering||Assignee:||Chris Dumez <cdumez>|
|Severity:||Normal||CC:||commit-queue, simon.fraser, thorton, zalan|
|Version:||528+ (Nightly build)|
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 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.