Re: [patch] libstdc++/61143 make unordered containers usable after move

[Jonathan Wakely <jwakely@redhat.com>]

On 19/05/14 22:27 +0200, François Dumont wrote:
>On 15/05/2014 22:52, Jonathan Wakely wrote:
>>Does this get initialized in the constructors?
>>Would it make sense to give it an initializer?
>>
>>     __bucket_type        _M_single_bucket = nullptr;
>
>    This bucket is replacing those normally allocated and when they 
>are allocated they are 0 initialised. So, you were right, there were 
>one place where this initialization was missing which is fixed in this 
>new patch. So I don't think this additional initialization is 
>necessary.

OK

>>>@@ -980,12 +999,16 @@
>>>    _M_move_assign(_Hashtable&& __ht, std::true_type)
>>>    {
>>>      this->_M_deallocate_nodes(_M_begin());
>>>-      if (__builtin_expect(_M_bucket_count != 0, true))
>>>-    _M_deallocate_buckets();
>>>-
>>>+      _M_deallocate_buckets();
>>>      __hashtable_base::operator=(std::move(__ht));
>>>      _M_rehash_policy = __ht._M_rehash_policy;
>>>-      _M_buckets = __ht._M_buckets;
>>>+      if (__builtin_expect(__ht._M_buckets != 
>>>&__ht._M_single_bucket, true))
>>>+    _M_buckets = __ht._M_buckets;
>>
>>What is the value of this->_M_single_bucket now?
>>
>>Should it be set to nullptr, if only to help debugging?
>
>We are not resetting buckets to null when rehashing so unless I add 
>more checks I won't be able to reset it each time.

OK

>>
>>>+      if (__builtin_expect(__ht._M_buckets == 
>>>&__ht._M_single_bucket, false))
>>
>>This check appears in a few places, I wonder if it is worth creating a
>>private member function to hide the details:
>>
>> bool _M_moved_from() const noexcept
>> {
>>   return __builtin_expect(_M_buckets == &_M_single_bucket, false);
>> }
>>
>>Then just test if (__ht._M_moved_from())
>>
>>Usually I would think the __builtin_expect should not be inside the
>>member function, so the caller decides what the expected result is,
>>but I think in all cases the result is expected to be false. That
>>matches how move semantics are designed: the object that gets moved
>>from is expected to be going out of scope, and so will be reused in a
>>minority of cases.
>>
>>>@@ -1139,7 +1170,14 @@
>>>    {
>>>      if (__ht._M_node_allocator() == this->_M_node_allocator())
>>>    {
>>>-      _M_buckets = __ht._M_buckets;
>>>+      if (__builtin_expect(__ht._M_buckets == 
>>>&__ht._M_single_bucket, false))
>>
>>This could be if (__ht._M_moved_from())
>
>I hesitated in doing so and finally do so. I only prefer 
>_M_use_single_bucket as we might not limit its usage to moved 
>instances.

Good point.

I think it should be called _M_uses_single_bucket() or
_M_using_single_bucket() though, otherwise it sounds more like it
answers the question "should I use a single bucket?" rather than "am I
using the single bucket?"

>>How about removing the std::swap(_M_buckets, __x._M_buckets) above and
>>doing (untested):
>>
>> if (this->_M_moved_from())
>>   {
>>     if (__x._M_moved_from())
>>       _M_buckets = &_M_single_bucket;
>>     else
>>       _M_buckets = __x._M_buckets;
>>     __x._M_buckets = &__x._M_single_bucket;
>>   }
>> else if (__x._M_moved_from())
>>   {
>>     __x._M_buckets = _M_buckets;
>>     _M_buckets = &_M_single_bucket;
>>   }
>> else
>>   std::swap(_M_buckets, __x._M_buckets);
>>
>>Is that worth it?  I'm not sure.
>
>Yes, with the newly introduced _M_use_single_bucket it worth it I 
>think and is what I have done.

OK. My sketch above avoided calling _M_moved_from() more than once per
object, but the compiler should be able to optimise your version to
avoid multiple calls anyway.

>Here is the new patch limited to what I really want to commit this time.

Great. Please commit to trunk and the 4.9 branch - thanks!