Bug 145369 – Clean up HashTable constructors

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Zan Dobersek]

Clean up HashTable constructors

[Zan Dobersek]

Created attachment 253686 [details]
Patch

[Zan Dobersek]

Comment on attachment 253686 [details]
Patch

Clearing flags on attachment: 253686

Committed r184949: <http://trac.webkit.org/changeset/184949>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Zan Dobersek]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145458

[Darin Adler]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Darin Adler]

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!