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

[Jonathan Wakely]

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!