Clean up HashTable constructors
Created attachment 253686 [details] Patch
Comment on attachment 253686 [details] Patch Clearing flags on attachment: 253686 Committed r184949: <http://trac.webkit.org/changeset/184949>
All reviewed patches have been landed. Closing bug.
Comment on attachment 253686 [details] 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?
Comment on attachment 253686 [details] 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.
Re-opened since this is blocked by bug 145458
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.
Comment on attachment 253686 [details] 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.
Comment on attachment 253686 [details] 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.
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.
Comment on attachment 253686 [details] 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!