From: Phil Willoughby <philwill@fb.com>
Date: Mon, 11 Sep 2017 19:20:45 +0000 (-0700)
Subject: Dead shift in ConcurrentHashMapSegment
X-Git-Tag: v2017.09.18.00~19
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=d25d17e7f426bd585d5975bc184aa3c26eb6c6ae;p=folly.git

Dead shift in ConcurrentHashMapSegment

Summary: Original problem detected by compiling with `-Werror,-Wunused-value`. On further inspection the only place which uses this detail class ensures that the `max_size` parameter is a power of two already, so we can discard the logic to manipulate `max_size` and put a `CHECK` clause in its place to guard against future changes to the caller that break this assumption.

Reviewed By: yfeldblum

Differential Revision: D5799110

fbshipit-source-id: d21ed9ff196d54ef91e38254df8b1b88bbf29275
---

diff --git a/folly/concurrency/ConcurrentHashMap.h b/folly/concurrency/ConcurrentHashMap.h
index 0bc4d16b..f261ac98 100644
--- a/folly/concurrency/ConcurrentHashMap.h
+++ b/folly/concurrency/ConcurrentHashMap.h
@@ -467,7 +467,7 @@ class ConcurrentHashMap {
     auto seg = segments_[i].load(std::memory_order_acquire);
     if (!seg) {
       auto newseg = (SegmentT*)Allocator().allocate(sizeof(SegmentT));
-      new (newseg)
+      newseg = new (newseg)
           SegmentT(size_ >> ShardBits, load_factor_, max_size_ >> ShardBits);
       if (!segments_[i].compare_exchange_strong(seg, newseg)) {
         // seg is updated with new value, delete ours.
diff --git a/folly/concurrency/detail/ConcurrentHashMap-detail.h b/folly/concurrency/detail/ConcurrentHashMap-detail.h
index ccef419d..7b9da9c0 100644
--- a/folly/concurrency/detail/ConcurrentHashMap-detail.h
+++ b/folly/concurrency/detail/ConcurrentHashMap-detail.h
@@ -215,17 +215,13 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment {
       size_t initial_buckets,
       float load_factor,
       size_t max_size)
-      : load_factor_(load_factor) {
+      : load_factor_(load_factor), max_size_(max_size) {
     auto buckets = (Buckets*)Allocator().allocate(sizeof(Buckets));
     initial_buckets = folly::nextPowTwo(initial_buckets);
-    if (max_size != 0) {
-      max_size_ = folly::nextPowTwo(max_size);
-    }
-    if (max_size_ > max_size) {
-      max_size_ >> 1;
-    }
-
-    CHECK(max_size_ == 0 || (folly::popcount(max_size_ - 1) + ShardBits <= 32));
+    DCHECK(
+        max_size_ == 0 ||
+        (isPowTwo(max_size_) &&
+         (folly::popcount(max_size_ - 1) + ShardBits <= 32)));
     new (buckets) Buckets(initial_buckets);
     buckets_.store(buckets, std::memory_order_release);
     load_factor_nodes_ = initial_buckets * load_factor_;
@@ -734,7 +730,7 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment {
   float load_factor_;
   size_t load_factor_nodes_;
   size_t size_{0};
-  size_t max_size_{0};
+  size_t const max_size_;
   Atom<Buckets*> buckets_{nullptr};
   Mutex m_;
 };