147446 2015-07-30 10:21:41 -0700 Minimize children vector capacity changes in PlatformCALayerRemote::recursiveBuildTransaction() 2015-07-30 11:37:11 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez commit-queue simon.fraser thorton zalan oldest_to_newest 1113395 0 cdumez 2015-07-30 10:21:41 -0700 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 1113398 1 257839 cdumez 2015-07-30 10:23:59 -0700 Created attachment 257839 Patch 1113404 2 257839 simon.fraser 2015-07-30 10:44:17 -0700 Comment on attachment 257839 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? 1113414 3 257839 cdumez 2015-07-30 11:10:45 -0700 Comment on attachment 257839 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. 1113437 4 257839 commit-queue 2015-07-30 11:37:06 -0700 Comment on attachment 257839 Patch Clearing flags on attachment: 257839 Committed r187594: <http://trac.webkit.org/changeset/187594> 1113438 5 commit-queue 2015-07-30 11:37:11 -0700 All reviewed patches have been landed. Closing bug. 257839 2015-07-30 10:23:59 -0700 2015-07-30 11:37:06 -0700 Patch bug-147446-20150730102358.patch text/plain 2231 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMTg3NTIzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggOTUxNTUzOGYzOTI4NDFl YjNiMzY2MDVjOGVlZjNhNDc5NDQ3ZjVjNS4uZjA2MWFhMzJlNDc0Mjc4YzAwMTE1NjE3MDNmNjE0 OTY3NTExMTQ4YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE1LTA3LTMwICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgTWluaW1pemUgY2hpbGRyZW4g dmVjdG9yIGNhcGFjaXR5IGNoYW5nZXMgaW4gUGxhdGZvcm1DQUxheWVyUmVtb3RlOjpyZWN1cnNp dmVCdWlsZFRyYW5zYWN0aW9uKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTE0NzQ0NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIE1pbmltaXplIGNoaWxkcmVuIHZlY3RvciBjYXBhY2l0eSBjaGFuZ2VzIGlu IFBsYXRmb3JtQ0FMYXllclJlbW90ZTo6cmVjdXJzaXZlQnVpbGRUcmFuc2FjdGlvbigpCisgICAg ICAgIGJ5IGxldmVyYWdpbmcgdGhlIGZhY3QgdGhhdCB3ZSBrbm93IGluIGFkdmFuY2UgaG93IG1h bnkgbGF5ZXIgSURzIGFyZSBnb2luZyB0byBiZSBhZGRlZC4gV2UKKyAgICAgICAgbm93IHNldCB0 aGUgVmVjdG9yIHNpemUgZnJvbSB0aGUgc3RhcnQsIGluc3RlYWQgb2YgY2xlYXJpbmcgaXQgYW5k IHRoZW4gZ3Jvd2luZyB0aGUgdmVjdG9yCisgICAgICAgIGNhcGFjaXR5IHdoZW4gYXBwZW5kaW5n IHRoZSBsYXllciBJRHMgb25lIGJ5IG9uZS4KKworICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFn ZS9tYWMvUGxhdGZvcm1DQUxheWVyUmVtb3RlLmNwcDoKKyAgICAgICAgKFdlYktpdDo6UGxhdGZv cm1DQUxheWVyUmVtb3RlOjpyZWN1cnNpdmVCdWlsZFRyYW5zYWN0aW9uKToKKwogMjAxNS0wNy0y OCAgU2FpZCBBYm91LUhhbGxhd2EgIDxzYWJvdWhhbGxhd2FAYXBwbGUuY29tPgogCiAgICAgICAg IFtpT1NdIFJFR1JFU1NJT04ocjE2ODA3NSk6IEZ1bGxzY3JlZW4gd2ViIHZpZGVvIGRvZXNuJ3Qg cGF1c2Ugb24gc2NyZWVuIGxvY2sKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nl c3MvV2ViUGFnZS9tYWMvUGxhdGZvcm1DQUxheWVyUmVtb3RlLmNwcCBiL1NvdXJjZS9XZWJLaXQy L1dlYlByb2Nlc3MvV2ViUGFnZS9tYWMvUGxhdGZvcm1DQUxheWVyUmVtb3RlLmNwcAppbmRleCA3 NmQzNTdhMmM5NDIyZjgzMTVhZjBiOWI0ZGMyOTEyNzM4MjBhZGJkLi45ZTFhM2EzM2I3ZWYxNjU0 ZjQ4ZGUxODkzNWY1MTYyYzI2NWY2NjhjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQ cm9jZXNzL1dlYlBhZ2UvbWFjL1BsYXRmb3JtQ0FMYXllclJlbW90ZS5jcHAKKysrIGIvU291cmNl L1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL21hYy9QbGF0Zm9ybUNBTGF5ZXJSZW1vdGUuY3Bw CkBAIC0xNTgsOSArMTU4LDkgQEAgdm9pZCBQbGF0Zm9ybUNBTGF5ZXJSZW1vdGU6OnJlY3Vyc2l2 ZUJ1aWxkVHJhbnNhY3Rpb24oUmVtb3RlTGF5ZXJUcmVlQ29udGV4dCYgY28KIAogICAgIGlmICht X3Byb3BlcnRpZXMuY2hhbmdlZFByb3BlcnRpZXMgIT0gUmVtb3RlTGF5ZXJUcmVlVHJhbnNhY3Rp b246Ok5vQ2hhbmdlKSB7CiAgICAgICAgIGlmIChtX3Byb3BlcnRpZXMuY2hhbmdlZFByb3BlcnRp ZXMgJiBSZW1vdGVMYXllclRyZWVUcmFuc2FjdGlvbjo6Q2hpbGRyZW5DaGFuZ2VkKSB7Ci0gICAg ICAgICAgICBtX3Byb3BlcnRpZXMuY2hpbGRyZW4uY2xlYXIoKTsKLSAgICAgICAgICAgIGZvciAo Y29uc3QgYXV0byYgbGF5ZXIgOiBtX2NoaWxkcmVuKQotICAgICAgICAgICAgICAgIG1fcHJvcGVy dGllcy5jaGlsZHJlbi5hcHBlbmQobGF5ZXItPmxheWVySUQoKSk7CisgICAgICAgICAgICBtX3By b3BlcnRpZXMuY2hpbGRyZW4ucmVzaXplKG1fY2hpbGRyZW4uc2l6ZSgpKTsKKyAgICAgICAgICAg IGZvciAoc2l6ZV90IGkgPSAwOyBpIDwgbV9jaGlsZHJlbi5zaXplKCk7ICsraSkKKyAgICAgICAg ICAgICAgICBtX3Byb3BlcnRpZXMuY2hpbGRyZW5baV0gPSBtX2NoaWxkcmVuW2ldLT5sYXllcklE KCk7CiAgICAgICAgIH0KIAogICAgICAgICBpZiAoaXNQbGF0Zm9ybUNBTGF5ZXJSZW1vdGVDdXN0 b20oKSkgewo=