145369 2015-05-25 10:20:18 -0700 Clean up HashTable constructors 2015-11-30 09:37:55 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 145458 1 zan zan benjamin cmarcelo commit-queue darin oldest_to_newest 1097246 0 zan 2015-05-25 10:20:18 -0700 Clean up HashTable constructors 1097248 1 253686 zan 2015-05-25 10:28:35 -0700 Created attachment 253686 Patch 1097864 2 253686 zan 2015-05-28 01:13:29 -0700 Comment on attachment 253686 Patch Clearing flags on attachment: 253686 Committed r184949: <http://trac.webkit.org/changeset/184949> 1097865 3 zan 2015-05-28 01:13:40 -0700 All reviewed patches have been landed. Closing bug. 1097921 4 253686 darin 2015-05-28 11:57:46 -0700 Comment on attachment 253686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253686&action=review > Source/WTF/wtf/HashTable.h:541 > - : m_table(0) > + : m_table(nullptr) > , m_tableSize(0) > , m_tableSizeMask(0) > , m_keyCount(0) > , m_deletedCount(0) > #if CHECK_HASHTABLE_ITERATORS > - , m_iterators(0) > + , m_iterators(nullptr) I think an even better cleanup is to initialize these where they are defined instead of listing them in the constructor. > Source/WTF/wtf/HashTable.h:1201 > template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits> > inline HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::HashTable(HashTable&& other) > -#if CHECK_HASHTABLE_ITERATORS > - : m_iterators(nullptr) > - , m_mutex(std::make_unique<std::mutex>()) > -#endif > + : HashTable() > { > - other.invalidateIterators(); > - > - m_table = other.m_table; > - m_tableSize = other.m_tableSize; > - m_tableSizeMask = other.m_tableSizeMask; > - m_keyCount = other.m_keyCount; > - m_deletedCount = other.m_deletedCount; > - > - other.m_table = nullptr; > - other.m_tableSize = 0; > - other.m_tableSizeMask = 0; > - other.m_keyCount = 0; > - other.m_deletedCount = 0; > - > -#if DUMP_HASHTABLE_STATS_PER_TABLE > - m_stats = WTF::move(other.m_stats); > - other.m_stats = nullptr; > -#endif > + swap(other); > } Is there any performance difference? 1098110 5 253686 zan 2015-05-29 08:07:53 -0700 Comment on attachment 253686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253686&action=review >> Source/WTF/wtf/HashTable.h:1201 >> } > > Is there any performance difference? The generated code is significantly larger, so I'll be rolling this out. 1098112 6 commit-queue 2015-05-29 08:14:14 -0700 Re-opened since this is blocked by bug 145458 1098123 7 darin 2015-05-29 09:19:45 -0700 Another way to "clean up" the HashTable move assignment operator is to use std::exchange instead of using separate assignment statements and nulling for each data member. 1120870 8 253686 zan 2015-08-26 00:05:33 -0700 Comment on attachment 253686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253686&action=review >>> Source/WTF/wtf/HashTable.h:1201 >>> } >> >> Is there any performance difference? > > The generated code is significantly larger, so I'll be rolling this out. Calling swap() in the move constructor instead of manually nulling out all the members on the 'other' object disables using SIMD registers, bloating up the generated code. Will avoid that. 1144776 9 253686 zan 2015-11-30 09:25:44 -0800 Comment on attachment 253686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253686&action=review >> Source/WTF/wtf/HashTable.h:541 >> + , m_iterators(nullptr) > > I think an even better cleanup is to initialize these where they are defined instead of listing them in the constructor. As it stands, the compilers generate the first initialization even if the member variable is reassigned in the actual constructor. So in the copy and move constructors some of these variables would be at first initialized, and then immediately reassigned with new values. 1144777 10 zan 2015-11-30 09:27:53 -0800 While the proposed changes would clean up the constructor code to some degree, the size of the generated code would increase as well. Given that this is utility-level library code, I think the verbosity should be preferred in this case. 1144783 11 253686 darin 2015-11-30 09:37:55 -0800 Comment on attachment 253686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253686&action=review >>> Source/WTF/wtf/HashTable.h:541 >>> + , m_iterators(nullptr) >> >> I think an even better cleanup is to initialize these where they are defined instead of listing them in the constructor. > > As it stands, the compilers generate the first initialization even if the member variable is reassigned in the actual constructor. So in the copy and move constructors some of these variables would be at first initialized, and then immediately reassigned with new values. No dead store optimization? That’s quite surprising! 253686 2015-05-25 10:28:35 -0700 2015-05-28 01:13:29 -0700 Patch bug-145369-20150525192824.patch text/plain 3787 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMTg0ODUxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDA1YWM5MmM3YTEwOGZkNDkzNDFiODcx YTQ2ODY1YWM1OGJjNDJjYzcuLmI5YjAwNDA1ZWFiNDVhNGQxNjc4MWJiMDFlOTNmNjRhYzZkMDA1 ZmMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTUtMDUtMjUgIFphbiBEb2JlcnNlayAgPHpkb2Jl cnNla0BpZ2FsaWEuY29tPgorCisgICAgICAgIENsZWFuIHVwIEhhc2hUYWJsZSBjb25zdHJ1Y3Rv cnMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0NTM2 OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVzZSBu dWxscHRyIHRvIGluaXRpYWxpemUgcG9pbnRlciBtZW1iZXIgdmFyaWFibGVzIGluIHRoZSBIYXNo VGFibGUKKyAgICAgICAgZGVmYXVsdCBjb25zdHJ1Y3Rvci4gQ29weSBhbmQgbW92ZSBjb25zdHJ1 Y3RvcnMgY2FuIHVzZSBjb25zdHJ1Y3RvcgorICAgICAgICBkZWxlZ2F0aW9uIGluc3RlYWQgb2Yg cmVwbGljYXRpbmcgYWxsIG1lbWJlciBpbml0aWFsaXphdGlvbnMuIE1vdmUKKyAgICAgICAgY29u c3RydWN0b3Igc2hvdWxkIHNpbXBseSBjYWxsIEhhc2hUYWJsZTo6c3dhcCgpIGluc3RlYWQgb2Yg cmVwbGljYXRpbmcKKyAgICAgICAgYWxsIHRoZSBzd2FwIG9wZXJhdGlvbnMuCisKKyAgICAgICAg KiB3dGYvSGFzaFRhYmxlLmg6CisgICAgICAgIChXVEY6OktleVRyYWl0cz46Okhhc2hUYWJsZSk6 CisKIDIwMTUtMDUtMjMgIERhbiBCZXJuc3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4KIAogICAgICAg ICBSZW1vdmUgdW51c2VkIGRlZmluaXRpb25zIG9mIFdFQktJVF9WRVJTSU9OX01JTl9SRVFVSVJF RApkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvSGFzaFRhYmxlLmggYi9Tb3VyY2UvV1RGL3d0 Zi9IYXNoVGFibGUuaAppbmRleCAxNDU0NTEyMmQ1NWM2NzBlZTkxNWNmNjQ0ZDU0OWU2NDRlZWE4 NDhiLi41NDZmYzRkMjEwMmNmNTkxNWEyMGU2NDg2MmM1MmZiMjNjY2M2ODJlIDEwMDY0NAotLS0g YS9Tb3VyY2UvV1RGL3d0Zi9IYXNoVGFibGUuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9IYXNoVGFi bGUuaApAQCAtNTMyLDEzICs1MzIsMTMgQEAgbmFtZXNwYWNlIFdURiB7CiAKICAgICB0ZW1wbGF0 ZTx0eXBlbmFtZSBLZXksIHR5cGVuYW1lIFZhbHVlLCB0eXBlbmFtZSBFeHRyYWN0b3IsIHR5cGVu YW1lIEhhc2hGdW5jdGlvbnMsIHR5cGVuYW1lIFRyYWl0cywgdHlwZW5hbWUgS2V5VHJhaXRzPgog ICAgIGlubGluZSBIYXNoVGFibGU8S2V5LCBWYWx1ZSwgRXh0cmFjdG9yLCBIYXNoRnVuY3Rpb25z LCBUcmFpdHMsIEtleVRyYWl0cz46Okhhc2hUYWJsZSgpCi0gICAgICAgIDogbV90YWJsZSgwKQor ICAgICAgICA6IG1fdGFibGUobnVsbHB0cikKICAgICAgICAgLCBtX3RhYmxlU2l6ZSgwKQogICAg ICAgICAsIG1fdGFibGVTaXplTWFzaygwKQogICAgICAgICAsIG1fa2V5Q291bnQoMCkKICAgICAg ICAgLCBtX2RlbGV0ZWRDb3VudCgwKQogI2lmIENIRUNLX0hBU0hUQUJMRV9JVEVSQVRPUlMKLSAg ICAgICAgLCBtX2l0ZXJhdG9ycygwKQorICAgICAgICAsIG1faXRlcmF0b3JzKG51bGxwdHIpCiAg ICAgICAgICwgbV9tdXRleChzdGQ6Om1ha2VfdW5pcXVlPHN0ZDo6bXV0ZXg+KCkpCiAjZW5kaWYK ICNpZiBEVU1QX0hBU0hUQUJMRV9TVEFUU19QRVJfVEFCTEUKQEAgLTExNTUsMTggKzExNTUsNyBA QCBuYW1lc3BhY2UgV1RGIHsKIAogICAgIHRlbXBsYXRlPHR5cGVuYW1lIEtleSwgdHlwZW5hbWUg VmFsdWUsIHR5cGVuYW1lIEV4dHJhY3RvciwgdHlwZW5hbWUgSGFzaEZ1bmN0aW9ucywgdHlwZW5h bWUgVHJhaXRzLCB0eXBlbmFtZSBLZXlUcmFpdHM+CiAgICAgSGFzaFRhYmxlPEtleSwgVmFsdWUs IEV4dHJhY3RvciwgSGFzaEZ1bmN0aW9ucywgVHJhaXRzLCBLZXlUcmFpdHM+OjpIYXNoVGFibGUo Y29uc3QgSGFzaFRhYmxlJiBvdGhlcikKLSAgICAgICAgOiBtX3RhYmxlKDApCi0gICAgICAgICwg bV90YWJsZVNpemUoMCkKLSAgICAgICAgLCBtX3RhYmxlU2l6ZU1hc2soMCkKLSAgICAgICAgLCBt X2tleUNvdW50KDApCi0gICAgICAgICwgbV9kZWxldGVkQ291bnQoMCkKLSNpZiBDSEVDS19IQVNI VEFCTEVfSVRFUkFUT1JTCi0gICAgICAgICwgbV9pdGVyYXRvcnMoMCkKLSAgICAgICAgLCBtX211 dGV4KHN0ZDo6bWFrZV91bmlxdWU8c3RkOjptdXRleD4oKSkKLSNlbmRpZgotI2lmIERVTVBfSEFT SFRBQkxFX1NUQVRTX1BFUl9UQUJMRQotICAgICAgICAsIG1fc3RhdHMoc3RkOjptYWtlX3VuaXF1 ZTxTdGF0cz4oKm90aGVyLm1fc3RhdHMpKQotI2VuZGlmCisgICAgICAgIDogSGFzaFRhYmxlKCkK ICAgICB7CiAgICAgICAgIC8vIENvcHkgdGhlIGhhc2ggdGFibGUgdGhlIGR1bWIgd2F5LCBieSBh ZGRpbmcgZWFjaCBlbGVtZW50IHRvIHRoZSBuZXcgdGFibGUuCiAgICAgICAgIC8vIEl0IG1pZ2h0 IGJlIG1vcmUgZWZmaWNpZW50IHRvIGNvcHkgdGhlIHRhYmxlIHNsb3RzLCBidXQgaXQncyBub3Qg Y2xlYXIgdGhhdCBlZmZpY2llbmN5IGlzIG5lZWRlZC4KQEAgLTEyMDYsMjkgKzExOTUsOSBAQCBu YW1lc3BhY2UgV1RGIHsKIAogICAgIHRlbXBsYXRlPHR5cGVuYW1lIEtleSwgdHlwZW5hbWUgVmFs dWUsIHR5cGVuYW1lIEV4dHJhY3RvciwgdHlwZW5hbWUgSGFzaEZ1bmN0aW9ucywgdHlwZW5hbWUg VHJhaXRzLCB0eXBlbmFtZSBLZXlUcmFpdHM+CiAgICAgaW5saW5lIEhhc2hUYWJsZTxLZXksIFZh bHVlLCBFeHRyYWN0b3IsIEhhc2hGdW5jdGlvbnMsIFRyYWl0cywgS2V5VHJhaXRzPjo6SGFzaFRh YmxlKEhhc2hUYWJsZSYmIG90aGVyKQotI2lmIENIRUNLX0hBU0hUQUJMRV9JVEVSQVRPUlMKLSAg ICAgICAgOiBtX2l0ZXJhdG9ycyhudWxscHRyKQotICAgICAgICAsIG1fbXV0ZXgoc3RkOjptYWtl X3VuaXF1ZTxzdGQ6Om11dGV4PigpKQotI2VuZGlmCisgICAgICAgIDogSGFzaFRhYmxlKCkKICAg ICB7Ci0gICAgICAgIG90aGVyLmludmFsaWRhdGVJdGVyYXRvcnMoKTsKLQotICAgICAgICBtX3Rh YmxlID0gb3RoZXIubV90YWJsZTsKLSAgICAgICAgbV90YWJsZVNpemUgPSBvdGhlci5tX3RhYmxl U2l6ZTsKLSAgICAgICAgbV90YWJsZVNpemVNYXNrID0gb3RoZXIubV90YWJsZVNpemVNYXNrOwot ICAgICAgICBtX2tleUNvdW50ID0gb3RoZXIubV9rZXlDb3VudDsKLSAgICAgICAgbV9kZWxldGVk Q291bnQgPSBvdGhlci5tX2RlbGV0ZWRDb3VudDsKLQotICAgICAgICBvdGhlci5tX3RhYmxlID0g bnVsbHB0cjsKLSAgICAgICAgb3RoZXIubV90YWJsZVNpemUgPSAwOwotICAgICAgICBvdGhlci5t X3RhYmxlU2l6ZU1hc2sgPSAwOwotICAgICAgICBvdGhlci5tX2tleUNvdW50ID0gMDsKLSAgICAg ICAgb3RoZXIubV9kZWxldGVkQ291bnQgPSAwOwotCi0jaWYgRFVNUF9IQVNIVEFCTEVfU1RBVFNf UEVSX1RBQkxFCi0gICAgICAgIG1fc3RhdHMgPSBXVEY6Om1vdmUob3RoZXIubV9zdGF0cyk7Ci0g ICAgICAgIG90aGVyLm1fc3RhdHMgPSBudWxscHRyOwotI2VuZGlmCisgICAgICAgIHN3YXAob3Ro ZXIpOwogICAgIH0KIAogICAgIHRlbXBsYXRlPHR5cGVuYW1lIEtleSwgdHlwZW5hbWUgVmFsdWUs IHR5cGVuYW1lIEV4dHJhY3RvciwgdHlwZW5hbWUgSGFzaEZ1bmN0aW9ucywgdHlwZW5hbWUgVHJh aXRzLCB0eXBlbmFtZSBLZXlUcmFpdHM+Cg==