Bug 145369 - Clean up HashTable constructors
Summary: Clean up HashTable constructors
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 145458
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-25 10:20 PDT by Zan Dobersek
Modified: 2015-11-30 09:37 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.70 KB, patch)
2015-05-25 10:28 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2015-05-25 10:20:18 PDT
Clean up HashTable constructors
Comment 1 Zan Dobersek 2015-05-25 10:28:35 PDT
Created attachment 253686 [details]
Patch
Comment 2 Zan Dobersek 2015-05-28 01:13:29 PDT
Comment on attachment 253686 [details]
Patch

Clearing flags on attachment: 253686

Committed r184949: <http://trac.webkit.org/changeset/184949>
Comment 3 Zan Dobersek 2015-05-28 01:13:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2015-05-28 11:57:46 PDT
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 5 Zan Dobersek 2015-05-29 08:07:53 PDT
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.
Comment 6 WebKit Commit Bot 2015-05-29 08:14:14 PDT
Re-opened since this is blocked by bug 145458
Comment 7 Darin Adler 2015-05-29 09:19:45 PDT
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 8 Zan Dobersek 2015-08-26 00:05:33 PDT
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 9 Zan Dobersek 2015-11-30 09:25:44 PST
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.
Comment 10 Zan Dobersek 2015-11-30 09:27:53 PST
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 11 Darin Adler 2015-11-30 09:37:55 PST
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!