165675 2016-12-09 12:37:22 -0800 Avoid calling shrink() in the Vector destructor 2016-12-10 10:04:45 -0800 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez andersca benjamin cmarcelo commit-queue darin dbates sam oldest_to_newest 1258246 0 cdumez 2016-12-09 12:37:22 -0800 Avoid calling shrink() in the Vector destructor to avoid function call overhead and unnecessarily reseting m_size to 0. 1258247 1 296674 cdumez 2016-12-09 12:38:12 -0800 Created attachment 296674 Patch 1258248 2 296674 cdumez 2016-12-09 12:41:01 -0800 Comment on attachment 296674 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296674&action=review > Source/WTF/wtf/Vector.h:633 > asanSetBufferSizeToFullCapacity(); Hmm, asanSetBufferSizeToFullCapacity() seems to rely on size() :/ 1258251 3 296676 cdumez 2016-12-09 12:44:53 -0800 Created attachment 296676 Patch 1258260 4 dbates 2016-12-09 12:55:56 -0800 How muck overhead are we talking about? Would to make sense to mark shrink() as inline or always inline? 1258262 5 dbates 2016-12-09 12:58:12 -0800 Better questions, what kind of speed up (percent of PLT) can we get with this change? 1258264 6 cdumez 2016-12-09 12:58:32 -0800 (In reply to comment #4) > How muck overhead are we talking about? Would to make sense to mark shrink() > as inline or always inline? Enough to show on speedometer profiles. I do not have have evidence shrink() needs to be inlined at other call sites. Also, merely inlining shrink() would still reset m_size to 0 unnecessarily in the destructor. 1258265 7 dbates 2016-12-09 13:01:45 -0800 (In reply to comment #6) > (In reply to comment #4) > > How muck overhead are we talking about? Would to make sense to mark shrink() > > as inline or always inline? > > Enough to show on speedometer profiles. I do not have have evidence shrink() > needs to be inlined at other call sites. Also, merely inlining shrink() > would still reset m_size to 0 unnecessarily in the destructor. Would it improve performance if shrink() was inlined at all callsites? 1258274 8 296676 cdumez 2016-12-09 13:23:55 -0800 Comment on attachment 296676 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296676&action=review > Source/WTF/wtf/Vector.h:631 > + TypeOperations::destruct(begin(), end()); Daniel suggested offline to add a comment explaining why we don't call shrink(0), worrying that someone might undo the optimization in the future. 1258277 9 296676 dbates 2016-12-09 13:28:19 -0800 Comment on attachment 296676 Patch I am surprised that function call overhead and setting m_size causes a significant performance degradation. It would nice be able to know how much of a speed up this change has or whether inlining shrink(0) everywhere would further improve performance and avoid special casing the destructor. 1258285 10 dbates 2016-12-09 13:36:17 -0800 (In reply to comment #9) > [...] It would nice be able to know how much > of a speed up this change has or whether inlining shrink(0) everywhere would > further improve performance and avoid special casing the destructor. This should read: It would be nice to know and document how much of a speedup this change has or whether inlining shrink() everywhere would further improve performance and avoid the need to special case the logic in the destructor. 1258432 11 296730 cdumez 2016-12-09 16:02:39 -0800 Created attachment 296730 Patch 1258442 12 cdumez 2016-12-09 16:19:14 -0800 (In reply to comment #11) > Created attachment 296730 [details] > Patch I think this is a bit nicer. 1258463 13 296730 dbates 2016-12-09 16:40:22 -0800 Comment on attachment 296730 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296730&action=review > Source/WTF/wtf/Vector.h:633 > - shrink(0); > + TypeOperations::destruct(begin(), end()); > > - asanSetBufferSizeToFullCapacity(); > + asanSetBufferSizeToFullCapacity(0); I still suggest that we add a comment to explain that the destructor is written this way to avoid the call to shrink(0) as a performance optimization to avoid a person from undoing this change. 1258573 14 296730 darin 2016-12-09 21:12:24 -0800 Comment on attachment 296730 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296730&action=review >> Source/WTF/wtf/Vector.h:633 >> + asanSetBufferSizeToFullCapacity(0); > > I still suggest that we add a comment to explain that the destructor is written this way to avoid the call to shrink(0) as a performance optimization to avoid a person from undoing this change. I’m not sure that’s needed. There are lots of different ways to write ~Vector less efficiently; I’m not sure that using shrink(0) is particularly more likely than the others. I am thinking that a comment saying “this is written the way it is as a performance optimization” is not really specific enough, and a comment saying “don’t use shrink(0) here” is too specific. 1258583 15 dbates 2016-12-09 21:29:31 -0800 (In reply to comment #14) > Comment on attachment 296730 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296730&action=review > > >> Source/WTF/wtf/Vector.h:633 > >> + asanSetBufferSizeToFullCapacity(0); > > > > I still suggest that we add a comment to explain that the destructor is written this way to avoid the call to shrink(0) as a performance optimization to avoid a person from undoing this change. > > I’m not sure that’s needed. There are lots of different ways to write > ~Vector less efficiently; I’m not sure that using shrink(0) is particularly > more likely than the others. I am thinking that a comment saying “this is > written the way it is as a performance optimization” is not really specific > enough, and a comment saying “don’t use shrink(0) here” is too specific. Fair enough. 1258699 16 296730 commit-queue 2016-12-10 10:04:40 -0800 Comment on attachment 296730 Patch Clearing flags on attachment: 296730 Committed r209664: <http://trac.webkit.org/changeset/209664> 1258700 17 commit-queue 2016-12-10 10:04:45 -0800 All reviewed patches have been landed. Closing bug. 296674 2016-12-09 12:38:12 -0800 2016-12-09 12:44:49 -0800 Patch bug-165675-20161209123730.patch text/plain 1172 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjA5NjIwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDYzNmVhZTJhOGE0MmZjNDAwYTUxYzg0 MDlmODBhY2Q2ZmQzY2RmNTQuLjcxNTNkZTUyZTM0MWRmZjViNGU3ODc1MTVlMmYzZjk1ODU1YTE2 ZWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTYtMTItMDkgIENocmlzIER1bWV6ICA8Y2R1bWV6 QGFwcGxlLmNvbT4KKworICAgICAgICBBdm9pZCBjYWxsaW5nIHNocmluaygpIGluIHRoZSBWZWN0 b3IgZGVzdHJ1Y3RvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTY1Njc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgQXZvaWQgY2FsbGluZyBzaHJpbmsoKSBpbiB0aGUgVmVjdG9yIGRlc3RydWN0b3IgdG8g YXZvaWQgZnVuY3Rpb24gY2FsbAorICAgICAgICBvdmVyaGVhZCBhbmQgdW5uZWNlc3NhcmlseSBy ZXNldGluZyBtX3NpemUgdG8gMC4KKworICAgICAgICAqIHd0Zi9WZWN0b3IuaDoKKyAgICAgICAg KFdURjo6VmVjdG9yOjp+VmVjdG9yKToKKwogMjAxNi0xMi0wOSAgR2VvZmZyZXkgR2FyZW4gIDxn Z2FyZW5AYXBwbGUuY29tPgogCiAgICAgICAgIE1vdmVkIE9yZGluYWxOdW1iZXIgaW50byBpdHMg b3duIGZpbGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL1ZlY3Rvci5oIGIvU291cmNlL1dU Ri93dGYvVmVjdG9yLmgKaW5kZXggNDliNDY5ODgxYzEyM2M1OTNmNTg0NjE4YmQ0NDE3NWY1NGQ1 MDQ2Yi4uZmNlZjZiMWY4ZjY1MDk4ODkyMGNkMTYwMzRkMGQ5MWQyNDBkMzI0NSAxMDA2NDQKLS0t IGEvU291cmNlL1dURi93dGYvVmVjdG9yLmgKKysrIGIvU291cmNlL1dURi93dGYvVmVjdG9yLmgK QEAgLTYyOCw3ICs2MjgsNyBAQCBwdWJsaWM6CiAgICAgflZlY3RvcigpCiAgICAgewogICAgICAg ICBpZiAobV9zaXplKQotICAgICAgICAgICAgc2hyaW5rKDApOworICAgICAgICAgICAgVHlwZU9w ZXJhdGlvbnM6OmRlc3RydWN0KGJlZ2luKCksIGVuZCgpKTsKIAogICAgICAgICBhc2FuU2V0QnVm ZmVyU2l6ZVRvRnVsbENhcGFjaXR5KCk7CiAgICAgfQo= 296676 2016-12-09 12:44:53 -0800 2016-12-09 16:02:34 -0800 Patch bug-165675-20161209124411.patch text/plain 1262 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjA5NjIwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDYzNmVhZTJhOGE0MmZjNDAwYTUxYzg0 MDlmODBhY2Q2ZmQzY2RmNTQuLjcxNTNkZTUyZTM0MWRmZjViNGU3ODc1MTVlMmYzZjk1ODU1YTE2 ZWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTYtMTItMDkgIENocmlzIER1bWV6ICA8Y2R1bWV6 QGFwcGxlLmNvbT4KKworICAgICAgICBBdm9pZCBjYWxsaW5nIHNocmluaygpIGluIHRoZSBWZWN0 b3IgZGVzdHJ1Y3RvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTY1Njc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgQXZvaWQgY2FsbGluZyBzaHJpbmsoKSBpbiB0aGUgVmVjdG9yIGRlc3RydWN0b3IgdG8g YXZvaWQgZnVuY3Rpb24gY2FsbAorICAgICAgICBvdmVyaGVhZCBhbmQgdW5uZWNlc3NhcmlseSBy ZXNldGluZyBtX3NpemUgdG8gMC4KKworICAgICAgICAqIHd0Zi9WZWN0b3IuaDoKKyAgICAgICAg KFdURjo6VmVjdG9yOjp+VmVjdG9yKToKKwogMjAxNi0xMi0wOSAgR2VvZmZyZXkgR2FyZW4gIDxn Z2FyZW5AYXBwbGUuY29tPgogCiAgICAgICAgIE1vdmVkIE9yZGluYWxOdW1iZXIgaW50byBpdHMg b3duIGZpbGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL1ZlY3Rvci5oIGIvU291cmNlL1dU Ri93dGYvVmVjdG9yLmgKaW5kZXggNDliNDY5ODgxYzEyM2M1OTNmNTg0NjE4YmQ0NDE3NWY1NGQ1 MDQ2Yi4uNDU5Y2NkMjRhNTViN2YwNjc3NGVkMjQyZGM3YjUyZGFlMmIxZWZkZiAxMDA2NDQKLS0t IGEvU291cmNlL1dURi93dGYvVmVjdG9yLmgKKysrIGIvU291cmNlL1dURi93dGYvVmVjdG9yLmgK QEAgLTYyNyw5ICs2MjcsMTIgQEAgcHVibGljOgogCiAgICAgflZlY3RvcigpCiAgICAgewotICAg ICAgICBpZiAobV9zaXplKQotICAgICAgICAgICAgc2hyaW5rKDApOwotCisgICAgICAgIGlmICht X3NpemUpIHsKKyAgICAgICAgICAgIFR5cGVPcGVyYXRpb25zOjpkZXN0cnVjdChiZWdpbigpLCBl bmQoKSk7CisjaWYgQVNBTl9FTkFCTEVECisgICAgICAgICAgICBtX3NpemUgPSAwOworI2VuZGlm CisgICAgICAgIH0KICAgICAgICAgYXNhblNldEJ1ZmZlclNpemVUb0Z1bGxDYXBhY2l0eSgpOwog ICAgIH0KIAo= 296730 2016-12-09 16:02:39 -0800 2016-12-10 10:04:40 -0800 Patch bug-165675-20161209160156.patch text/plain 2616 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjA5NjI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDYzNmVhZTJhOGE0MmZjNDAwYTUxYzg0 MDlmODBhY2Q2ZmQzY2RmNTQuLjdmMWFiMDNlYTY1MzE0NjI3N2RkYjQ0YzNhOTQwNjJlYzNjMGVk MTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTYtMTItMDkgIENocmlzIER1bWV6ICA8Y2R1bWV6 QGFwcGxlLmNvbT4KKworICAgICAgICBBdm9pZCBjYWxsaW5nIHNocmluaygpIGluIHRoZSBWZWN0 b3IgZGVzdHJ1Y3RvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTY1Njc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgQXZvaWQgY2FsbGluZyBzaHJpbmsoKSBpbiB0aGUgVmVjdG9yIGRlc3RydWN0b3IgdG8g YXZvaWQgZnVuY3Rpb24gY2FsbAorICAgICAgICBvdmVyaGVhZCBhbmQgdW5uZWNlc3NhcmlseSBy ZXNldGluZyBtX3NpemUgdG8gMC4KKworICAgICAgICAqIHd0Zi9WZWN0b3IuaDoKKyAgICAgICAg KFdURjo6VmVjdG9yOjp+VmVjdG9yKToKKyAgICAgICAgKFdURjo6VmVjdG9yOjphc2FuU2V0QnVm ZmVyU2l6ZVRvRnVsbENhcGFjaXR5KToKKyAgICAgICAgKFdURjo6bWluQ2FwYWNpdHk+Ojphc2Fu U2V0QnVmZmVyU2l6ZVRvRnVsbENhcGFjaXR5KToKKwogMjAxNi0xMi0wOSAgR2VvZmZyZXkgR2Fy ZW4gIDxnZ2FyZW5AYXBwbGUuY29tPgogCiAgICAgICAgIE1vdmVkIE9yZGluYWxOdW1iZXIgaW50 byBpdHMgb3duIGZpbGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL1ZlY3Rvci5oIGIvU291 cmNlL1dURi93dGYvVmVjdG9yLmgKaW5kZXggNDliNDY5ODgxYzEyM2M1OTNmNTg0NjE4YmQ0NDE3 NWY1NGQ1MDQ2Yi4uNmZkZDE5YWQ0OTVjM2Y2MDc2ZDI3MjAwZjRhZjg0OTNjYmEyNDM1MyAxMDA2 NDQKLS0tIGEvU291cmNlL1dURi93dGYvVmVjdG9yLmgKKysrIGIvU291cmNlL1dURi93dGYvVmVj dG9yLmgKQEAgLTYyOCw5ICs2MjgsOSBAQCBwdWJsaWM6CiAgICAgflZlY3RvcigpCiAgICAgewog ICAgICAgICBpZiAobV9zaXplKQotICAgICAgICAgICAgc2hyaW5rKDApOworICAgICAgICAgICAg VHlwZU9wZXJhdGlvbnM6OmRlc3RydWN0KGJlZ2luKCksIGVuZCgpKTsKIAotICAgICAgICBhc2Fu U2V0QnVmZmVyU2l6ZVRvRnVsbENhcGFjaXR5KCk7CisgICAgICAgIGFzYW5TZXRCdWZmZXJTaXpl VG9GdWxsQ2FwYWNpdHkoMCk7CiAgICAgfQogCiAgICAgVmVjdG9yKGNvbnN0IFZlY3RvciYpOwpA QCAtNzg5LDcgKzc4OSw5IEBAIHByaXZhdGU6CiAgICAgdGVtcGxhdGU8dHlwZW5hbWUuLi4gQXJn cz4gYm9vbCB0cnlDb25zdHJ1Y3RBbmRBcHBlbmRTbG93Q2FzZShBcmdzJiYuLi4pOwogCiAgICAg dm9pZCBhc2FuU2V0SW5pdGlhbEJ1ZmZlclNpemVUbyhzaXplX3QpOwotICAgIHZvaWQgYXNhblNl dEJ1ZmZlclNpemVUb0Z1bGxDYXBhY2l0eSgpOworICAgIHZvaWQgYXNhblNldEJ1ZmZlclNpemVU b0Z1bGxDYXBhY2l0eShzaXplX3QpOworICAgIHZvaWQgYXNhblNldEJ1ZmZlclNpemVUb0Z1bGxD YXBhY2l0eSgpIHsgYXNhblNldEJ1ZmZlclNpemVUb0Z1bGxDYXBhY2l0eShzaXplKCkpOyB9CisK ICAgICB2b2lkIGFzYW5CdWZmZXJTaXplV2lsbENoYW5nZVRvKHNpemVfdCk7CiAKICAgICB1c2lu ZyBCYXNlOjptX3NpemU7CkBAIC0xMDU2LDE0ICsxMDU4LDE2IEBAIGlubGluZSB2b2lkIFZlY3Rv cjxULCBpbmxpbmVDYXBhY2l0eSwgT3ZlcmZsb3dIYW5kbGVyLCBtaW5DYXBhY2l0eT46OmFzYW5T ZXRJbml0CiB9CiAKIHRlbXBsYXRlPHR5cGVuYW1lIFQsIHNpemVfdCBpbmxpbmVDYXBhY2l0eSwg dHlwZW5hbWUgT3ZlcmZsb3dIYW5kbGVyLCBzaXplX3QgbWluQ2FwYWNpdHk+Ci1pbmxpbmUgdm9p ZCBWZWN0b3I8VCwgaW5saW5lQ2FwYWNpdHksIE92ZXJmbG93SGFuZGxlciwgbWluQ2FwYWNpdHk+ Ojphc2FuU2V0QnVmZmVyU2l6ZVRvRnVsbENhcGFjaXR5KCkKK2lubGluZSB2b2lkIFZlY3RvcjxU LCBpbmxpbmVDYXBhY2l0eSwgT3ZlcmZsb3dIYW5kbGVyLCBtaW5DYXBhY2l0eT46OmFzYW5TZXRC dWZmZXJTaXplVG9GdWxsQ2FwYWNpdHkoc2l6ZV90IHNpemUpCiB7CiAjaWYgQVNBTl9FTkFCTEVE CiAgICAgaWYgKCFidWZmZXIoKSkKICAgICAgICAgcmV0dXJuOwogCiAgICAgLy8gQVNhbiByZXF1 aXJlcyB0aGF0IHRoZSBhbm5vdGF0aW9uIGlzIHJldHVybmVkIHRvIGl0cyBpbml0aWFsIHN0YXRl IGJlZm9yZSBkZWFsbG9jYXRpb24uCi0gICAgX19zYW5pdGl6ZXJfYW5ub3RhdGVfY29udGlndW91 c19jb250YWluZXIoYnVmZmVyKCksIGVuZE9mQnVmZmVyKCksIGJ1ZmZlcigpICsgc2l6ZSgpLCBl bmRPZkJ1ZmZlcigpKTsKKyAgICBfX3Nhbml0aXplcl9hbm5vdGF0ZV9jb250aWd1b3VzX2NvbnRh aW5lcihidWZmZXIoKSwgZW5kT2ZCdWZmZXIoKSwgYnVmZmVyKCkgKyBzaXplLCBlbmRPZkJ1ZmZl cigpKTsKKyNlbHNlCisgICAgVU5VU0VEX1BBUkFNKHNpemUpOwogI2VuZGlmCiB9CiAK