MB-24055: remove defaultNum{Buckets,Locks}, use config.json instead 86/77886/5
authorDave Rigby <daver@couchbase.com>
Tue, 9 May 2017 12:20:31 +0000 (13:20 +0100)
committerDave Rigby <daver@couchbase.com>
Fri, 12 May 2017 09:07:28 +0000 (09:07 +0000)
HashTable defines it's own set of default values for the hash table
initial size and number of locks. This appears to be a remnant of a
time before the central configuration.json we now know and love.

Having both is over-complex and confusing - it might /appear/ that the
default ht_size is 47 (from configuration.json), but in fact the
default is 3 - as hard-coded in hash_table.cc

Given we have config.json now, just use that as the holder of the
default sizes (and which can be overridden by ns_server if
desired). As such HashTable now requires an explicit initialSize and
nLocks in the constructor - which in normal usage will simply come
from the config.

Note that there should be no material change in the values used for
ht_locks or ht_size when run as a full Couchbase Server instance - if
a value is passed in by ns_server (via bucket config string) that will
be used; otherwise the previous values of defaultNum{Buckets,Locks} -
3 and 193 respectively - are used. In unit tests, any previously
overridden values have been updated here to the same values.

Change-Id: I8e3067014714a48acf1954310fdf13dce0b731d7
Reviewed-on: http://review.couchbase.org/77886
Reviewed-by: James Harrison <james.harrison@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: Build Bot <build@couchbase.com>
benchmarks/access_scanner_bench.cc
configuration.json
src/ep_engine.cc
src/hash_table.cc
src/hash_table.h
src/vbucket.cc
tests/module_tests/hash_table_test.cc
tests/module_tests/item_pager_test.cc
tests/module_tests/stored_value_test.cc

index 050a2fa..52e69e1 100644 (file)
@@ -32,7 +32,7 @@ protected:
         memoryTracker = BenchmarkMemoryTracker::getInstance(
                 *get_mock_server_api()->alloc_hooks);
         memoryTracker->reset();
-        std::string config = "dbname=benchmarks-test;" + varConfig;
+        std::string config = "dbname=benchmarks-test;ht_locks=47;" + varConfig;
 
         engine.reset(new SynchronousEPEngine(config));
         ObjectRegistry::onSwitchThread(engine.get());
index f90692d..0caf807 100644 (file)
             "type": "size_t"
         },
         "ht_size": {
-            "default": "0",
+            "default": "3",
+            "descr": "Initial number of slots in HashTable objects.",
             "type": "size_t"
         },
         "initfile": {
index 97c2c15..3b23521 100644 (file)
@@ -1909,8 +1909,6 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::initialize(const char* config) {
     maxFailoverEntries = configuration.getMaxFailoverEntries();
 
     // Start updating the variables from the config!
-    HashTable::setDefaultNumBuckets(configuration.getHtSize());
-    HashTable::setDefaultNumLocks(configuration.getHtLocks());
     StoredValue::setMutationMemoryThreshold(
                                       configuration.getMutationMemThreshold());
 
index 2af8867..d02d8f0 100644 (file)
 
 #include <cstring>
 
-// Given we default to 1024 vBuckets, set default HT buckets to lowest
-// prime in our table - this still gives space for 3072 HT slots but
-// minimizes fixed overheads.
-size_t HashTable::defaultNumBuckets = 3;
-size_t HashTable::defaultNumLocks = 47;
-
-static ssize_t prime_size_table[] = {
+static const ssize_t prime_size_table[] = {
     3, 7, 13, 23, 47, 97, 193, 383, 769, 1531, 3079, 6143, 12289, 24571, 49157,
     98299, 196613, 393209, 786433, 1572869, 3145721, 6291449, 12582917,
     25165813, 50331653, 100663291, 201326611, 402653189, 805306357,
@@ -42,7 +36,8 @@ std::ostream& operator<<(std::ostream& os, const HashTable::Position& pos) {
 
 HashTable::HashTable(EPStats& st,
                      std::unique_ptr<AbstractStoredValueFactory> svFactory,
-                     size_t s, size_t l)
+                     size_t initialSize,
+                     size_t locks)
     : maxDeletedRevSeqno(0),
       numTotalItems(0),
       numNonResidentItems(0),
@@ -52,14 +47,15 @@ HashTable::HashTable(EPStats& st,
       memSize(0),
       cacheSize(0),
       metaDataMemory(0),
+      initialSize(initialSize),
+      size(initialSize),
+      n_locks(locks),
       stats(st),
       valFact(std::move(svFactory)),
       visitors(0),
       numItems(0),
       numResizes(0),
       numTempItems(0) {
-    size = HashTable::getNumBuckets(s);
-    n_locks = HashTable::getNumLocks(l);
     values.resize(size);
     mutexes = new std::mutex[n_locks];
     activeState = true;
@@ -147,9 +143,9 @@ void HashTable::resize() {
     if (prime_size_table[i] == -1) {
         // We're at the end, take the biggest
         new_size = prime_size_table[i-1];
-    } else if (prime_size_table[i] < static_cast<ssize_t>(defaultNumBuckets)) {
-        // Was going to be smaller than the configured ht_size.
-        new_size = defaultNumBuckets;
+    } else if (prime_size_table[i] < static_cast<ssize_t>(initialSize)) {
+        // Was going to be smaller than the initial size.
+        new_size = initialSize;
     } else if (0 == i) {
         new_size = prime_size_table[i];
     }else if (isCurrently(size, prime_size_table[i-1], prime_size_table[i])) {
@@ -621,30 +617,6 @@ HashTable::Position HashTable::endPosition() const  {
     return HashTable::Position(size, n_locks, size);
 }
 
-static inline size_t getDefault(size_t x, size_t d) {
-    return x == 0 ? d : x;
-}
-
-size_t HashTable::getNumBuckets(size_t n) {
-    return getDefault(n, defaultNumBuckets);
-}
-
-size_t HashTable::getNumLocks(size_t n) {
-    return getDefault(n, defaultNumLocks);
-}
-
-void HashTable::setDefaultNumBuckets(size_t to) {
-    if (to != 0) {
-        defaultNumBuckets = to;
-    }
-}
-
-void HashTable::setDefaultNumLocks(size_t to) {
-    if (to != 0) {
-        defaultNumLocks = to;
-    }
-}
-
 bool HashTable::unlocked_ejectItem(StoredValue*& vptr,
                                    item_eviction_policy_t policy) {
     if (vptr == nullptr) {
index 53a48e7..ce17847 100644 (file)
@@ -168,13 +168,13 @@ public:
      *
      * @param st the global stats reference
      * @param svFactory Factory to use for constructing stored values
-     * @param s the number of hash table buckets
-     * @param l the number of locks in the hash table
+     * @param initialSize the number of hash table buckets to initially create.
+     * @param locks the number of locks in the hash table
      */
     HashTable(EPStats& st,
               std::unique_ptr<AbstractStoredValueFactory> svFactory,
-              size_t s = 0,
-              size_t l = 0);
+              size_t initialSize,
+              size_t locks);
 
     ~HashTable();
 
@@ -493,16 +493,6 @@ public:
     static size_t getNumLocks(size_t s);
 
     /**
-     * Set the default number of buckets.
-     */
-    static void setDefaultNumBuckets(size_t);
-
-    /**
-     * Set the default number of locks.
-     */
-    static void setDefaultNumLocks(size_t);
-
-    /**
      * Get the max deleted revision seqno seen so far.
      */
     uint64_t getMaxDeletedRevSeqno() const {
@@ -605,6 +595,9 @@ private:
     inline bool isActive() const { return activeState; }
     inline void setActiveState(bool newv) { activeState = newv; }
 
+    // The initial (and minimum) size of the HashTable.
+    const size_t initialSize;
+
     std::atomic<size_t> size;
     size_t               n_locks;
     table_type values;
@@ -617,9 +610,6 @@ private:
     std::atomic<size_t>       numTempItems;
     bool                 activeState;
 
-    static size_t                 defaultNumBuckets;
-    static size_t                 defaultNumLocks;
-
     int getBucketForHash(int h) {
         return abs(h % static_cast<int>(size));
     }
index 309a54b..93ab6d8 100644 (file)
@@ -150,7 +150,7 @@ VBucket::VBucket(id_type i,
                  uint64_t purgeSeqno,
                  uint64_t maxCas,
                  const std::string& collectionsManifest)
-    : ht(st, std::move(valFact)),
+    : ht(st, std::move(valFact), config.getHtSize(), config.getHtLocks()),
       checkpointManager(st,
                         i,
                         chkConfig,
index e4de0d3..4328026 100644 (file)
@@ -137,10 +137,11 @@ protected:
         }
         return std::make_unique<StoredValueFactory>(global_stats);
     }
+    const size_t defaultHtSize = Configuration().getHtSize();
 };
 
 TEST_F(HashTableTest, Size) {
-    HashTable h(global_stats, makeFactory(), /*size*/0, /*locks*/1);
+    HashTable h(global_stats, makeFactory(), defaultHtSize, /*locks*/ 1);
     ASSERT_EQ(0, count(h));
 
     store(h, makeStoredDocKey("testkey"));
@@ -149,7 +150,7 @@ TEST_F(HashTableTest, Size) {
 }
 
 TEST_F(HashTableTest, SizeTwo) {
-    HashTable h(global_stats, makeFactory(), /*size*/0, /*locks*/1);
+    HashTable h(global_stats, makeFactory(), defaultHtSize, /*locks*/ 1);
     ASSERT_EQ(0, count(h));
 
     auto keys = generateKeys(5);
index d39425c..a29a019 100644 (file)
@@ -66,7 +66,8 @@ public:
 
 protected:
     void SetUp() override {
-        config_string += "max_size=" + std::to_string(200 * 1024) +
+        // Set specific ht_size given we need to control expected memory usage.
+        config_string += "ht_size=47;max_size=" + std::to_string(200 * 1024) +
                          ";mem_low_wat=" + std::to_string(120 * 1024) +
                          ";mem_high_wat=" + std::to_string(160 * 1024);
         STParameterizedBucketTest::SetUp();
@@ -80,6 +81,9 @@ protected:
 
         // Sanity check - to ensure memory usage doesn't increase without us
         // noticing.
+        ASSERT_EQ(47, store->getVBucket(vbid)->ht.getSize())
+                << "Expected to have a HashTable of size 47 (mem calculations "
+                   "based on this).";
         auto& stats = engine->getEpStats();
         ASSERT_LE(stats.getTotalMemoryUsed(), 20 * 1024)
             << "Expected to start with less than 20KB of memory used";
index 78f10a2..289255a 100644 (file)
@@ -38,7 +38,7 @@ public:
     ValueTest()
         : stats(),
           factory(stats),
-          ht(stats, /*size:default*/ 0, /*locks*/ 1),
+          ht(stats, std::make_unique<Factory>(stats), /*size*/ 47, /*locks*/ 1),
           item(make_item(0, makeStoredDocKey("key"), "value")) {
     }