135638 2014-08-05 20:38:09 -0700 HashTable based classes leak a lot 2014-08-10 01:45:46 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 benjamin benjamin cmarcelo commit-queue darin oldest_to_newest 1027074 0 benjamin 2014-08-05 20:38:09 -0700 HashTable based classes leak a lot 1027076 1 236078 benjamin 2014-08-05 20:43:55 -0700 Created attachment 236078 Patch 1027078 2 236078 darin 2014-08-05 20:48:07 -0700 Comment on attachment 236078 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review Another fix would be to just add these two lines: if (m_table) deallocateTable(m_table, m_tableSize); I suppose the swap idiom is more elegant, but it would be nice to double check that it’s just as efficient too. > Source/WTF/wtf/HashTable.h:1236 > + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. 1027087 3 benjamin 2014-08-05 22:15:09 -0700 (In reply to comment #2) > (From update of attachment 236078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > Another fix would be to just add these two lines: > > if (m_table) > deallocateTable(m_table, m_tableSize); > > I suppose the swap idiom is more elegant, but it would be nice to double check that it’s just as efficient too. The previous version does not look like it was written to be efficient. It is zeroing the attributes that depends on the table being non null. ...but I'll check. 1027270 4 benjamin 2014-08-06 13:08:17 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 236078 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > > > Another fix would be to just add these two lines: > > > > if (m_table) > > deallocateTable(m_table, m_tableSize); > > > > I suppose the swap idiom is more elegant, but it would be nice to double check that it’s just as efficient too. > > The previous version does not look like it was written to be efficient. It is zeroing the attributes that depends on the table being non null. > > ...but I'll check. Ok, tried both on x86_64, the version with "if (m_table)" is significantly worse than the swap(). 1027271 5 236078 benjamin 2014-08-06 13:12:34 -0700 Comment on attachment 236078 Patch Clearing flags on attachment: 236078 Committed r172167: <http://trac.webkit.org/changeset/172167> 1027272 6 benjamin 2014-08-06 13:12:37 -0700 All reviewed patches have been landed. Closing bug. 1027861 7 236078 darin 2014-08-08 09:33:22 -0700 Comment on attachment 236078 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review >> Source/WTF/wtf/HashTable.h:1236 >> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); > > You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. Did you see this comment? I noticed you landed it with the long thing. 1027961 8 benjamin 2014-08-08 14:57:03 -0700 (In reply to comment #7) > (From update of attachment 236078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > >> Source/WTF/wtf/HashTable.h:1236 > >> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); > > > > You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. > > Did you see this comment? I noticed you landed it with the long thing. I am one of the dinosaurs who find code a lot harder to read with "auto" :) If you feel strongly about it, I'll update to auto. 1028025 9 236078 darin 2014-08-08 17:44:31 -0700 Comment on attachment 236078 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review >>>> Source/WTF/wtf/HashTable.h:1236 >>>> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); >>> >>> You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. >> >> Did you see this comment? I noticed you landed it with the long thing. > > I am one of the dinosaurs who find code a lot harder to read with "auto" :) > > If you feel strongly about it, I'll update to auto. Since you don’t like auto, you can do this: HashTable temp = WTF::move(other); To me it reads even better with auto, but I understand that you don’t like it. That’s why I said “just HashTable” *or* “just auto”. 1028028 10 benjamin 2014-08-08 18:00:09 -0700 (In reply to comment #9) > (From update of attachment 236078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > >>>> Source/WTF/wtf/HashTable.h:1236 > >>>> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); > >>> > >>> You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. > >> > >> Did you see this comment? I noticed you landed it with the long thing. > > > > I am one of the dinosaurs who find code a lot harder to read with "auto" :) > > > > If you feel strongly about it, I'll update to auto. > > Since you don’t like auto, you can do this: > > HashTable temp = WTF::move(other); > > To me it reads even better with auto, but I understand that you don’t like it. That’s why I said “just HashTable” *or* “just auto”. Oh, I misunderstood the part about HashTable. I agree HashTable is better, I will update. 1028087 11 benjamin 2014-08-10 01:45:46 -0700 Fixed the type in r172378: <http://trac.webkit.org/changeset/172378> 236078 2014-08-05 20:43:55 -0700 2014-08-08 17:44:31 -0700 Patch bug-135638-20140805204336.patch text/plain 2255 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTcyMDkzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDVlZWU5NDNkNjU1Y2NjNWY5NGY0M2Y1 MTZhNDNlYTA3M2NlY2E5Y2MuLjA4NDI3OWZkN2ZkMmI1NGFjMDQwYjllYzE0N2MyYjdiZGEyMDYy YzMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTQtMDgtMDUgIEJlbmphbWluIFBvdWxhaW4gIDxi ZW5qYW1pbkB3ZWJraXQub3JnPgorCisgICAgICAgIEhhc2hUYWJsZSBiYXNlZCBjbGFzc2VzIGxl YWsgYSBsb3QKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTEzNTYzOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg ICogd3RmL0hhc2hUYWJsZS5oOgorICAgICAgICBUaGUgb3BlcmF0b3I9IHRha2luZyBhIHJ2YWx1 ZSByZWZlcmVuY2Ugd2FzIG5ldmVyIGZyZWVpbmcgdGhlIG1lbW9yeSBhbGxvY2F0ZWQKKyAgICAg ICAgZm9yIHRoZSB0YWJsZSBvZiB0aGUgbGVmdCBoYW5kIHNpZGUgb2JqZWN0LgorCisgICAgICAg IFRoaXMgcGF0Y2ggZml4ZXMgdGhlIGxlYWtzIGJ5IGRvaW5nIGFuIGFsbG9jK3N3YXAgd2l0aCBh IG5ldyBvYmplY3QuCisgICAgICAgIFRoZSBvYmplY3QgdGVtcCBnZXRzIHRoZSByZWZlcmVuY2Ug dG8gbV90YWJsZSwgYW5kIGRlc3Ryb3lzIGl0IGluIHRoZSByZWd1bGFyIGRlc3RydWN0b3IKKyAg ICAgICAgd2hlbiBnb2luZyBvdXQgb2Ygc2NvcGUuCisKKyAgICAgICAgS3Vkb3MgdG8gUHJhdGlr IFNvbGFua2kgZm9yIGZpbmRpbmcgdGhlIGxlYWtzLgorCiAyMDE0LTA4LTA0ICBBbGV4IENocmlz dGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUHJvZ3Jlc3MgdG93 YXJkcyBDTWFrZSBvbiBNYWMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9IYXNoVGFibGUu aCBiL1NvdXJjZS9XVEYvd3RmL0hhc2hUYWJsZS5oCmluZGV4IDgzYzJkOTY2ODU2NTZjY2FiZmJk NmI3OTNmNDRkMDBlZGVhNTAyNzMuLjY0ZDI3MDVjZDJjN2I3ZjkzNDE2ZmYzODBmODY0ZWJkODk1 ZjQ0YmQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0hhc2hUYWJsZS5oCisrKyBiL1NvdXJj ZS9XVEYvd3RmL0hhc2hUYWJsZS5oCkBAIC0xMjMzLDI2ICsxMjMzLDggQEAgbmFtZXNwYWNlIFdU RiB7CiAgICAgdGVtcGxhdGU8dHlwZW5hbWUgS2V5LCB0eXBlbmFtZSBWYWx1ZSwgdHlwZW5hbWUg RXh0cmFjdG9yLCB0eXBlbmFtZSBIYXNoRnVuY3Rpb25zLCB0eXBlbmFtZSBUcmFpdHMsIHR5cGVu YW1lIEtleVRyYWl0cz4KICAgICBpbmxpbmUgYXV0byBIYXNoVGFibGU8S2V5LCBWYWx1ZSwgRXh0 cmFjdG9yLCBIYXNoRnVuY3Rpb25zLCBUcmFpdHMsIEtleVRyYWl0cz46Om9wZXJhdG9yPShIYXNo VGFibGUmJiBvdGhlcikgLT4gSGFzaFRhYmxlJgogICAgIHsKLSAgICAgICAgaW52YWxpZGF0ZUl0 ZXJhdG9ycygpOwotICAgICAgICBvdGhlci5pbnZhbGlkYXRlSXRlcmF0b3JzKCk7Ci0KLSAgICAg ICAgbV90YWJsZSA9IG90aGVyLm1fdGFibGU7Ci0gICAgICAgIG1fdGFibGVTaXplID0gb3RoZXIu bV90YWJsZVNpemU7Ci0gICAgICAgIG1fdGFibGVTaXplTWFzayA9IG90aGVyLm1fdGFibGVTaXpl TWFzazsKLSAgICAgICAgbV9rZXlDb3VudCA9IG90aGVyLm1fa2V5Q291bnQ7Ci0gICAgICAgIG1f ZGVsZXRlZENvdW50ID0gb3RoZXIubV9kZWxldGVkQ291bnQ7Ci0KLSAgICAgICAgb3RoZXIubV90 YWJsZSA9IG51bGxwdHI7Ci0gICAgICAgIG90aGVyLm1fdGFibGVTaXplID0gMDsKLSAgICAgICAg b3RoZXIubV90YWJsZVNpemVNYXNrID0gMDsKLSAgICAgICAgb3RoZXIubV9rZXlDb3VudCA9IDA7 Ci0gICAgICAgIG90aGVyLm1fZGVsZXRlZENvdW50ID0gMDsKLQotI2lmIERVTVBfSEFTSFRBQkxF X1NUQVRTX1BFUl9UQUJMRQotICAgICAgICBtX3N0YXRzID0gV1RGOjptb3ZlKG90aGVyLm1fc3Rh dHMpOwotICAgICAgICBvdGhlci5tX3N0YXRzID0gbnVsbHB0cjsKLSNlbmRpZgotCisgICAgICAg IEhhc2hUYWJsZTxLZXksIFZhbHVlLCBFeHRyYWN0b3IsIEhhc2hGdW5jdGlvbnMsIFRyYWl0cywg S2V5VHJhaXRzPiB0ZW1wID0gV1RGOjptb3ZlKG90aGVyKTsKKyAgICAgICAgc3dhcCh0ZW1wKTsK ICAgICAgICAgcmV0dXJuICp0aGlzOwogICAgIH0KIAo=