Re: [boost] GSOC 2015 : Project on Concurrent Hash Tables

Subject: Re: [boost] GSOC 2015 : Project on Concurrent Hash Tables
From: Rob Stewart (rob.stewart_at_[hidden])
Date: 2015-02-25 04:57:42

On February 24, 2015 10:00:36 PM EST, Amarnath V A <me_at_[hidden]> wrote:
> 
> +      concurrent_unordered_map(concurrent_unordered_map &&old_map)
> BOOST_NOEXCEPT : _hasher(std::move(old_map._hasher)),
> _key_equal(std::move(old_map._key_eq

I think you missed moving a couple of data members from old_map.

> +      {
> +       //Hold the rehash lock
> +        typedef decltype(_rehash_lock) rehash_lock_t;
> +        typedef decltype(old_map._rehash_lock) old_rehash_lock_t;
> +        lock_guard<rehash_lock_t> guard(_rehash_lock,
> adopt_lock_t());

It doesn't make sense to hold the lock for this during move construction. Since the new object does not yet exist, there can be no code using it, much less contending for it.

> +        lock_guard<old_rehash_lock_t> old_guard(old_map._rehash_lock,
> adopt_lock_t());

It doesn't make sense to me to acquire the lock for the old map during move construction since the old map is a temporary by definition.

> +       _buckets.exchange(old_map._buckets.load(memory_order_consume),
> memory_order_release);
> +        old_map._buckets.exchange(nullptr, memory_order_acq_rel);

Why would you do atomic exchanges after acquiring locks? Perhaps the locks are for another purpose, and atomicity is needed apart from the locks generally, but in the move constructor, that isn't needed.

> +       old_map._buckets = new buckets_type(13);

Why would you allocate new buckets for the old map? You should just be ensuring that old_map is destructible or assignable, and I imagine you can do that without allocating new memory.

> +       old_map._oldbucketit = old_map._oldbuckets.begin();
> +       old_map._oldbuckets.fill(nullptr);

Is that needed?

> +      }
> +
> +      concurrent_unordered_map &operator=(concurrent_unordered_map
> &&old_map) BOOST_NOEXCEPT
> +      {
> +       if (this != &old_map) {

Self move assignment would be pathological. Why not forego this and make it a precondition?

> +         //Hold the rehash lock
> +         typedef decltype(_rehash_lock) rehash_lock_t;
> +         typedef decltype(old_map._rehash_lock) old_rehash_lock_t;
> +         lock_guard<rehash_lock_t> guard(_rehash_lock,
> adopt_lock_t());

Can acquiring the lock throw? That would violate your noexcept declaration.

> +         lock_guard<old_rehash_lock_t>
> old_guard(old_map._rehash_lock,
> adopt_lock_t());

The old map is a temporary, so there's no need to acquire its lock.

> +  _buckets.exchange(old_map._buckets.load(memory_order_consume),
> memory_order_release);

Is atomicity really needed here?

> +         _hasher = std::move(old_map._hasher);
> +         _key_equal = std::move(old_map._key_equal);
> +         _max_load_factor = std::move(old_map._max_load_factor);
> +         _min_bucket_capacity =
> std::move(old_map._min_bucket_capacity);
> +         old_map._buckets.exchange(nullptr, memory_order_acq_rel);

Atomicity?

> +         old_map._buckets = new buckets_type(13);

Why allocate? Just ensure old_map is destructible or assignable. Also note that new can throw which would violate your noexcept declaration.

> +         old_map._oldbucketit = old_map._oldbuckets.begin();
> +         old_map._oldbuckets.fill(nullptr);

Is that needed?

___
Rob

(Sent from my portable computation engine)