115733 2013-05-07 10:06:25 -0700 Avoid unnecessary arguments copying inside GenericHashTraits methods 2013-05-08 13:55:47 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mikhail.pozdnyakov mikhail.pozdnyakov andersca benjamin cmarcelo commit-queue darin oldest_to_newest 885796 0 mikhail.pozdnyakov 2013-05-07 10:06:25 -0700 At the moment we have in GenericHashTraits: typedef T PassOutType; static PassOutType passOut(const T& value) { return value; } and typedef T PeekType; static PeekType peek(const T& value) { return value; } In case T is a class having non-trivial copy constructor it'll be invoked on exit of both passOut() and peek(). (even with O3 optimization enabled for GCC at least) 885800 1 200922 mikhail.pozdnyakov 2013-05-07 10:15:30 -0700 Created attachment 200922 patch 885933 2 200922 benjamin 2013-05-07 12:51:26 -0700 Comment on attachment 200922 patch I fail to see in which case this is relevant. Can you give more explanation in the ChangeLog? For this kind of patch, you should also include in the ChangeLog how much smaller the binary becomes. For changes in inlines function, the binary size often gives reviewers a good metric on how large the improvement is. 886251 3 mikhail.pozdnyakov 2013-05-08 02:32:12 -0700 (In reply to comment #2) > (From update of attachment 200922 [details]) > I fail to see in which case this is relevant. Can you give more explanation in the ChangeLog? > Sure, I'll provide more explanation. > For this kind of patch, you should also include in the ChangeLog how much smaller the binary becomes. For changes in inlines function, the binary size often gives reviewers a good metric on how large the improvement is. Thanks for an interesting metric proposal :) That is what 'ls' gave in the size column for libjavascriptcore_efl.so.0.1.0 (EFL release build): before my change: 6554992 after my change: 6554560 886266 4 201050 mikhail.pozdnyakov 2013-05-08 03:20:25 -0700 Created attachment 201050 patch v2 Improved change log. 886332 5 201050 darin 2013-05-08 07:26:04 -0700 Comment on attachment 201050 patch v2 Change seems OK. 886333 6 darin 2013-05-08 07:26:31 -0700 I believe newer compilers would optimize such temporaries out. I am really surprised the EFL compiler does not do so! 886339 7 mikhail.pozdnyakov 2013-05-08 07:39:02 -0700 (In reply to comment #6) > I believe newer compilers would optimize such temporaries out. I am really surprised the EFL compiler does not do so! That was gcc 4.6.3 886350 8 201050 commit-queue 2013-05-08 08:07:11 -0700 Comment on attachment 201050 patch v2 Clearing flags on attachment: 201050 Committed r149738: <http://trac.webkit.org/changeset/149738> 886351 9 commit-queue 2013-05-08 08:07:14 -0700 All reviewed patches have been landed. Closing bug. 886461 10 benjamin 2013-05-08 13:55:47 -0700 Thank you for updating the ChangeLog. 200922 2013-05-07 10:15:30 -0700 2013-05-08 03:20:25 -0700 patch bug115733 text/plain 2064 mikhail.pozdnyakov Y29tbWl0IDE4NDBmM2Y3MzRhNWRmYjhiZjBiYTQzMjY2ZmJjNGQwOTU4ZjMxMmIKQXV0aG9yOiBN aWtoYWlsIFBvemRueWFrb3YgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CkRhdGU6ICAg VHVlIE1heSA3IDIwOjEzOjIwIDIwMTMgKzAzMDAKCiAgICBidWcxMTU3MzMKCmRpZmYgLS1naXQg YS9Tb3VyY2UvV1RGL0NoYW5nZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDEwYzY4 ZTguLjk2Njk3M2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XVEYvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTggQEAKIDIwMTMtMDUtMDcgIE1pa2hhaWwgUG96 ZG55YWtvdiAgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CiAKKyAgICAgICAgQXZvaWQg dW5uZWNlc3NhcnkgYXJndW1lbnRzIGNvcHlpbmcgaW5zaWRlIEdlbmVyaWNIYXNoVHJhaXRzIG1l dGhvZHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEx NTczMworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog d3RmL0hhc2hUcmFpdHMuaDoKKyAgICAgICAgKFdURjo6R2VuZXJpY0hhc2hUcmFpdHM6OnBhc3NP dXQpOiAKKyAgICAgICAgKFdURjo6R2VuZXJpY0hhc2hUcmFpdHM6OnBlZWspOgorICAgICAgICAg ICAgQWRkZWQgb3ZlcmxvYWRlZCBmdW5jdGlvbnMgdGhhdCBhY2NlcHQgbm9uLXRlbXBvcmFyeSB2 YWx1ZXMKKyAgICAgICAgICAgIGFuZCBkbyBub3QgY29weSBhcmd1bWVudHMuCisKKzIwMTMtMDUt MDcgIE1pa2hhaWwgUG96ZG55YWtvdiAgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CisK ICAgICAgICAgSGFzaFRyYWl0czxSZWZQdHI8UD4gPjo6UGVla1R5cGUgc2hvdWxkIGJlIHJhdyBw b2ludGVyIGZvciBiZXR0ZXIgcGVyZm9ybWFuY2UKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTExNTY0NgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0 Zi9IYXNoVHJhaXRzLmggYi9Tb3VyY2UvV1RGL3d0Zi9IYXNoVHJhaXRzLmgKaW5kZXggMmJlMzNj Mi4uODUyYTdjOSAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvSGFzaFRyYWl0cy5oCisrKyBi L1NvdXJjZS9XVEYvd3RmL0hhc2hUcmFpdHMuaApAQCAtNzgsMTIgKzc4LDE0IEBAIG5hbWVzcGFj ZSBXVEYgewogICAgICAgICAvLyBUeXBlIGZvciByZXR1cm4gdmFsdWUgb2YgZnVuY3Rpb25zIHRo YXQgdHJhbnNmZXIgb3duZXJzaGlwLCBzdWNoIGFzIHRha2UuIAogICAgICAgICB0eXBlZGVmIFQg UGFzc091dFR5cGU7CiAgICAgICAgIHN0YXRpYyBQYXNzT3V0VHlwZSBwYXNzT3V0KGNvbnN0IFQm IHZhbHVlKSB7IHJldHVybiB2YWx1ZTsgfQorICAgICAgICBzdGF0aWMgVCYgcGFzc091dChUJiB2 YWx1ZSkgeyByZXR1cm4gdmFsdWU7IH0gLy8gT3ZlcmxvYWRlZCB0byBhdm9pZCBjb3B5aW5nIG9m IG5vbi10ZW1wb3JhcnkgdmFsdWVzLgogCiAgICAgICAgIC8vIFR5cGUgZm9yIHJldHVybiB2YWx1 ZSBvZiBmdW5jdGlvbnMgdGhhdCBkbyBub3QgdHJhbnNmZXIgb3duZXJzaGlwLCBzdWNoIGFzIGdl dC4KICAgICAgICAgLy8gRklYTUU6IFdlIGNvdWxkIGNoYW5nZSB0aGlzIHR5cGUgdG8gY29uc3Qg VCYgZm9yIGJldHRlciBwZXJmb3JtYW5jZSBpZiB3ZSBmaWd1cmVkIG91dAogICAgICAgICAvLyBh IHdheSB0byBoYW5kbGUgdGhlIHJldHVybiB2YWx1ZSBmcm9tIGVtcHR5VmFsdWUsIHdoaWNoIGlz IGEgdGVtcG9yYXJ5LgogICAgICAgICB0eXBlZGVmIFQgUGVla1R5cGU7CiAgICAgICAgIHN0YXRp YyBQZWVrVHlwZSBwZWVrKGNvbnN0IFQmIHZhbHVlKSB7IHJldHVybiB2YWx1ZTsgfQorICAgICAg ICBzdGF0aWMgVCYgcGVlayhUJiB2YWx1ZSkgeyByZXR1cm4gdmFsdWU7IH0gLy8gT3ZlcmxvYWRl ZCB0byBhdm9pZCBjb3B5aW5nIG9mIG5vbi10ZW1wb3JhcnkgdmFsdWVzLgogICAgIH07CiAKICAg ICB0ZW1wbGF0ZTx0eXBlbmFtZSBUPiBzdHJ1Y3QgSGFzaFRyYWl0cyA6IEdlbmVyaWNIYXNoVHJh aXRzPFQ+IHsgfTsK 201050 2013-05-08 03:20:25 -0700 2013-05-08 08:07:10 -0700 patch v2 bug115733_v2 text/plain 2740 mikhail.pozdnyakov Y29tbWl0IGEzNDc4MTczMDljMjA4NDhmNzU0MjcwMmYzZDVlZDYxYTMyNjcyNjIKQXV0aG9yOiBN aWtoYWlsIFBvemRueWFrb3YgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CkRhdGU6ICAg VHVlIE1heSA3IDIwOjEzOjIwIDIwMTMgKzAzMDAKCiAgICBidWcxMTU3MzMKCmRpZmYgLS1naXQg YS9Tb3VyY2UvV1RGL0NoYW5nZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDEwYzY4 ZTguLmY3N2YyMjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XVEYvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjggQEAKIDIwMTMtMDUtMDcgIE1pa2hhaWwgUG96 ZG55YWtvdiAgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CiAKKyAgICAgICAgQXZvaWQg dW5uZWNlc3NhcnkgYXJndW1lbnRzIGNvcHlpbmcgaW5zaWRlIEdlbmVyaWNIYXNoVHJhaXRzIG1l dGhvZHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEx NTczMworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEJl Zm9yZSB0aGUgY2hhbmdlIGJvdGggV1RGOjpHZW5lcmljSGFzaFRyYWl0czo6cGFzc091dCBhbmQg V1RGOjpHZW5lcmljSGFzaFRyYWl0czo6cGFzc091dAorICAgICAgICB1c2VkIHRvIHJldHVybiB0 aGUgZ2l2ZW4gYXJndW1lbnQgYWx3YXlzIGJ5IHZhbHVlIGFuZCB0aGF0IGNhdXNlZCBpbXBsaWNp dCBleHRyYQorICAgICAgICBjb3B5aW5nIG9mIHRoZSBhcmd1bWVudC4gSXQgd2FzIE9LIGFzIGxv bmcgYXMgYXJndW1lbnQgdHlwZSBUIHdhcyBQT0QsIGFzIGNvbXBpbGVyCisgICAgICAgIGNvdWxk IG9wdGltaXplIGl0LCBidXQgaW4gY2FzZSBUIHdhcyBhIGNsYXNzIGhhdmluZyBub24tdHJpdmlh bCBjb3B5IGNvbnN0cnVjdG9yIHRoZQorICAgICAgICBleHRyYSBjb3B5aW5nIG9mIHRoZSBhcmd1 bWVudCBjb3VsZCBub3QgaGF2ZSBiZWVuIG9idmlhdGVkLgorCisgICAgICAgIFRoZSBwcm9wb3Nl ZCBzb2x1dGlvbiBpcyB0byBwcm92aWRlIG92ZXJsb2FkZWQgZnVuY3Rpb25zIHRoYXQgYWNjZXB0 IG5vbi10ZW1wb3JhcnkKKyAgICAgICAgdmFsdWVzIGFuZCByZXR1cm4gdGhlbSBieSByZWZlcmVu Y2UgdGh1cyBhdm9pZGluZyBleHRyYSBjb3B5aW5nLgorCisgICAgICAgIFRoZSBwcm9wb3NlZCBz b2x1dGlvbiBtYWRlIGFuIGltcGFjdCBvbiB0aGUgc2l6ZSBvZiBsaWJqYXZhc2NyaXB0Y29yZV9l Zmwuc28gKEVGTAorICAgICAgICByZWxlYXNlIGJ1aWxkKTogdGhlIHNpemUgZGVjcmVhc2VkIGZy b20gNjU1NDk5MiBieXRlcyB0byA2NTU0NTYwIGJ5dGVzLgorCisgICAgICAgICogd3RmL0hhc2hU cmFpdHMuaDoKKyAgICAgICAgKFdURjo6R2VuZXJpY0hhc2hUcmFpdHM6OnBhc3NPdXQpOiAKKyAg ICAgICAgKFdURjo6R2VuZXJpY0hhc2hUcmFpdHM6OnBlZWspOgorCisyMDEzLTA1LTA3ICBNaWto YWlsIFBvemRueWFrb3YgIDxtaWtoYWlsLnBvemRueWFrb3ZAaW50ZWwuY29tPgorCiAgICAgICAg IEhhc2hUcmFpdHM8UmVmUHRyPFA+ID46OlBlZWtUeXBlIHNob3VsZCBiZSByYXcgcG9pbnRlciBm b3IgYmV0dGVyIHBlcmZvcm1hbmNlCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xMTU2NDYKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvSGFzaFRy YWl0cy5oIGIvU291cmNlL1dURi93dGYvSGFzaFRyYWl0cy5oCmluZGV4IDJiZTMzYzIuLjg1MmE3 YzkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0hhc2hUcmFpdHMuaAorKysgYi9Tb3VyY2Uv V1RGL3d0Zi9IYXNoVHJhaXRzLmgKQEAgLTc4LDEyICs3OCwxNCBAQCBuYW1lc3BhY2UgV1RGIHsK ICAgICAgICAgLy8gVHlwZSBmb3IgcmV0dXJuIHZhbHVlIG9mIGZ1bmN0aW9ucyB0aGF0IHRyYW5z ZmVyIG93bmVyc2hpcCwgc3VjaCBhcyB0YWtlLiAKICAgICAgICAgdHlwZWRlZiBUIFBhc3NPdXRU eXBlOwogICAgICAgICBzdGF0aWMgUGFzc091dFR5cGUgcGFzc091dChjb25zdCBUJiB2YWx1ZSkg eyByZXR1cm4gdmFsdWU7IH0KKyAgICAgICAgc3RhdGljIFQmIHBhc3NPdXQoVCYgdmFsdWUpIHsg cmV0dXJuIHZhbHVlOyB9IC8vIE92ZXJsb2FkZWQgdG8gYXZvaWQgY29weWluZyBvZiBub24tdGVt cG9yYXJ5IHZhbHVlcy4KIAogICAgICAgICAvLyBUeXBlIGZvciByZXR1cm4gdmFsdWUgb2YgZnVu Y3Rpb25zIHRoYXQgZG8gbm90IHRyYW5zZmVyIG93bmVyc2hpcCwgc3VjaCBhcyBnZXQuCiAgICAg ICAgIC8vIEZJWE1FOiBXZSBjb3VsZCBjaGFuZ2UgdGhpcyB0eXBlIHRvIGNvbnN0IFQmIGZvciBi ZXR0ZXIgcGVyZm9ybWFuY2UgaWYgd2UgZmlndXJlZCBvdXQKICAgICAgICAgLy8gYSB3YXkgdG8g aGFuZGxlIHRoZSByZXR1cm4gdmFsdWUgZnJvbSBlbXB0eVZhbHVlLCB3aGljaCBpcyBhIHRlbXBv cmFyeS4KICAgICAgICAgdHlwZWRlZiBUIFBlZWtUeXBlOwogICAgICAgICBzdGF0aWMgUGVla1R5 cGUgcGVlayhjb25zdCBUJiB2YWx1ZSkgeyByZXR1cm4gdmFsdWU7IH0KKyAgICAgICAgc3RhdGlj IFQmIHBlZWsoVCYgdmFsdWUpIHsgcmV0dXJuIHZhbHVlOyB9IC8vIE92ZXJsb2FkZWQgdG8gYXZv aWQgY29weWluZyBvZiBub24tdGVtcG9yYXJ5IHZhbHVlcy4KICAgICB9OwogCiAgICAgdGVtcGxh dGU8dHlwZW5hbWUgVD4gc3RydWN0IEhhc2hUcmFpdHMgOiBHZW5lcmljSGFzaFRyYWl0czxUPiB7 IH07Cg==