Remove hooksApi global; reduce coupling with MemoryTracker 10/68210/6
authorDave Rigby <daver@couchbase.com>
Fri, 12 Aug 2016 13:45:20 +0000 (14:45 +0100)
committerDave Rigby <daver@couchbase.com>
Tue, 11 Oct 2016 08:02:38 +0000 (08:02 +0000)
MemoryTracker is somewhat tightly coupled with ep_engine.cc as it uses
the getHooksApi() function to obtain the memory allocator hooks.

Firstly this makes it hard to test - compile one file and you have to
include the other, and it's also difficult to provide a different
hooks api - either for injecting a mock one for testing, or simply to
use the 'normal' hooks API but without pulling in ep-engine.

Secondly, there is unnecessary indirection in NewHook / DeleteHook -
which are called on every new/delete so performance is relevant
there. By giving the MemoryTracker it's own copy of the alloc_hooks
(instead of calling getHooksApi() on every call) we can reduce the
amount of work in NewHook / DeleteHook by approx. 50% (measured in
terms of x64 instructions).

Change-Id: Ia0f8ebb0a5263567dc08b32fe6ff9b7ea9eefa92
Reviewed-on: http://review.couchbase.org/68210
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
CMakeLists.txt
src/ep_engine.cc
src/memory_tracker.cc
src/memory_tracker.h
src/utility.h
tests/module_tests/defragmenter_test.cc
tests/module_tests/memory_tracker_test.cc
tests/module_tests/mock_hooks_api.cc
tests/module_tests/mock_hooks_api.h [new file with mode: 0644]

index 4f028cd..191e345 100644 (file)
@@ -383,9 +383,10 @@ ADD_EXECUTABLE(ep-engine_defragmenter_test
                src/vbucket.cc
                ${OBJECTREGISTRY_SOURCE}
                ${Memcached_SOURCE_DIR}/utilities/extension_loggers.c
+               ${Memcached_SOURCE_DIR}/programs/engine_testapp/mock_server.cc
                $<TARGET_OBJECTS:memory_tracking>)
 TARGET_LINK_LIBRARIES(ep-engine_defragmenter_test
-                      cJSON gtest platform ${MALLOC_LIBRARIES}
+                      cJSON gtest mcd_util platform ${MALLOC_LIBRARIES}
                       ${SNAPPY_LIBRARIES})
 
 ADD_LIBRARY(ep_testsuite SHARED
index dbd6a7a..b7461ff 100644 (file)
@@ -58,8 +58,6 @@
 #include "warmup.h"
 #include "string_utils.h"
 
-static AtomicValue<ALLOCATOR_HOOKS_API*> hooksApi;
-
 // The global logger instance, used by LOG() when components don't specify
 // their own more specific logger.
 static Logger global_logger;
@@ -1852,10 +1850,9 @@ extern "C" {
             return ENGINE_ENOTSUP;
         }
 
-        hooksApi.store(api->alloc_hooks, std::memory_order_relaxed);
         Logger::setLoggerAPI(api->log);
 
-        MemoryTracker::getInstance();
+        MemoryTracker::getInstance(*api->alloc_hooks);
         ObjectRegistry::initialize(api->alloc_hooks->get_allocation_size);
 
         AtomicValue<size_t>* inital_tracking = new AtomicValue<size_t>();
@@ -1963,9 +1960,6 @@ void LOG(EXTENSION_LOG_LEVEL severity, const char *fmt, ...) {
     va_end(va);
 }
 
-ALLOCATOR_HOOKS_API *getHooksApi(void) {
-    return hooksApi.load(std::memory_order_relaxed);
-}
 
 EventuallyPersistentEngine::EventuallyPersistentEngine(
                                     GET_SERVER_API get_server_api) :
@@ -3705,10 +3699,11 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::doMemoryStats(const void *cookie,
     add_casted_stat("ep_item_num", stats.numItem, add_stat, cookie);
 
     std::map<std::string, size_t> alloc_stats;
-    MemoryTracker::getInstance()->getAllocatorStats(alloc_stats);
-    std::map<std::string, size_t>::iterator it = alloc_stats.begin();
-    for (; it != alloc_stats.end(); ++it) {
-        add_casted_stat(it->first.c_str(), it->second, add_stat, cookie);
+    MemoryTracker::getInstance(*getServerApiFunc()->alloc_hooks)->
+        getAllocatorStats(alloc_stats);
+
+    for (const auto& it : alloc_stats) {
+        add_casted_stat(it.first.c_str(), it.second, add_stat, cookie);
     }
 
     return ENGINE_SUCCESS;
@@ -4758,7 +4753,8 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::getStats(const void* cookie,
         rv = ENGINE_SUCCESS;
     } else if (nkey == 9 && strncmp(stat_key, "allocator", 9) ==0) {
         char buffer[64 * 1024];
-        MemoryTracker::getInstance()->getDetailedStats(buffer, sizeof(buffer));
+        MemoryTracker::getInstance(*getServerApiFunc()->alloc_hooks)->
+                getDetailedStats(buffer, sizeof(buffer));
         add_casted_stat("detailed", buffer, add_stat, cookie);
         rv = ENGINE_SUCCESS;
     } else if (nkey == 6 && strncmp(stat_key, "config", 6) == 0) {
index be2faa4..4dfb7db 100644 (file)
@@ -47,7 +47,7 @@ void MemoryTracker::statsThreadMainLoop(void* arg) {
     }
 }
 
-MemoryTracker* MemoryTracker::getInstance() {
+MemoryTracker* MemoryTracker::getInstance(const ALLOCATOR_HOOKS_API& hooks_api_) {
     MemoryTracker* tmp = instance.load();
     if (tmp == nullptr) {
         // Double-checked locking if instance is null - ensure two threads
@@ -55,8 +55,15 @@ MemoryTracker* MemoryTracker::getInstance() {
         std::lock_guard<std::mutex> lock(instance_mutex);
         tmp = instance.load();
         if (tmp == nullptr) {
-            tmp = new MemoryTracker();
+            // Note that object construction and hook attaching is
+            // split. The issue is that until the constructor
+            // completes (and {instance} is assigned) it is not
+            // possible to 'service' any memory allocator callbacks -
+            // as the New/Delete hooks need to read {instance}.hooks_api
+            tmp = new MemoryTracker(hooks_api_);
             instance.store(tmp);
+
+            instance.load()->connectHooks();
         }
     }
     return tmp;
@@ -71,56 +78,62 @@ void MemoryTracker::destroyInstance() {
     }
 }
 
-extern "C" {
-    static void NewHook(const void* ptr, size_t) {
-        if (ptr != NULL) {
-            void* p = const_cast<void*>(ptr);
-            size_t alloc = getHooksApi()->get_allocation_size(p);
-            ObjectRegistry::memoryAllocated(alloc);
-        }
+void MemoryTracker::NewHook(const void* ptr, size_t) {
+    if (ptr != NULL) {
+        const auto* tracker = MemoryTracker::instance.load();
+        void* p = const_cast<void*>(ptr);
+        size_t alloc = tracker->hooks_api.get_allocation_size(p);
+        ObjectRegistry::memoryAllocated(alloc);
     }
+}
 
-    static void DeleteHook(const void* ptr) {
-        if (ptr != NULL) {
-            void* p = const_cast<void*>(ptr);
-            size_t alloc = getHooksApi()->get_allocation_size(p);
-            ObjectRegistry::memoryDeallocated(alloc);
-        }
+void MemoryTracker::DeleteHook(const void* ptr) {
+    if (ptr != NULL) {
+        const auto* tracker = MemoryTracker::instance.load();
+        void* p = const_cast<void*>(ptr);
+        size_t alloc = tracker->hooks_api.get_allocation_size(p);
+        ObjectRegistry::memoryDeallocated(alloc);
     }
 }
 
-MemoryTracker::MemoryTracker() {
+MemoryTracker::MemoryTracker(const ALLOCATOR_HOOKS_API& hooks_api_)
+    : hooks_api(hooks_api_) {
+    // Just create the object, actual hook registration happens
+    // once we have a concrete object constructed.
+}
+
+void MemoryTracker::connectHooks() {
     if (getenv("EP_NO_MEMACCOUNT") != NULL) {
         LOG(EXTENSION_LOG_NOTICE, "Memory allocation tracking disabled");
         return;
     }
-    stats.ext_stats_size = getHooksApi()->get_extra_stats_size();
+    stats.ext_stats_size = hooks_api.get_extra_stats_size();
     stats.ext_stats = new allocator_ext_stat[stats.ext_stats_size]();
 
-    if (getHooksApi()->add_new_hook(&NewHook)) {
+    if (hooks_api.add_new_hook(&NewHook)) {
         LOG(EXTENSION_LOG_DEBUG, "Registered add hook");
-        if (getHooksApi()->add_delete_hook(&DeleteHook)) {
+        if (hooks_api.add_delete_hook(&DeleteHook)) {
             LOG(EXTENSION_LOG_DEBUG, "Registered delete hook");
             std::cout.flush();
             tracking = true;
             updateStats();
             if (cb_create_named_thread(&statsThreadId,
                                        statsThreadMainLoop,
-                                           this, 0, "mc:mem stats") != 0) {
+                                       this, 0, "mc:mem stats") != 0) {
                 throw std::runtime_error(
-                                      "Error creating thread to update stats");
+                        "Error creating thread to update stats");
             }
             return;
         }
         std::cout.flush();
-        getHooksApi()->remove_new_hook(&NewHook);
+        hooks_api.remove_new_hook(&NewHook);
     }
     LOG(EXTENSION_LOG_WARNING, "Failed to register allocator hooks");
 }
 
 MemoryTracker::~MemoryTracker() {
-    getHooksApi()->remove_new_hook(&NewHook);
-    getHooksApi()->remove_delete_hook(&DeleteHook);
+    hooks_api.remove_new_hook(&NewHook);
+    hooks_api.remove_delete_hook(&DeleteHook);
     if (tracking) {
         tracking = false;
         shutdown_cv.notify_all();
@@ -154,11 +167,11 @@ void MemoryTracker::getAllocatorStats(std::map<std::string, size_t>
 }
 
 void MemoryTracker::getDetailedStats(char* buffer, int size) {
-    getHooksApi()->get_detailed_stats(buffer, size);
+    hooks_api.get_detailed_stats(buffer, size);
 }
 
 void MemoryTracker::updateStats() {
-    getHooksApi()->get_allocator_stats(&stats);
+    hooks_api.get_allocator_stats(&stats);
 }
 
 size_t MemoryTracker::getFragmentation() {
index ea4d284..5c0c364 100644 (file)
@@ -38,7 +38,13 @@ class MemoryTracker {
 public:
     ~MemoryTracker();
 
-    static MemoryTracker* getInstance();
+    /* Creates the singleton instance of the MemoryTracker (if it doesn't exist).
+     * Thread-safe, so ok for multiple threads to attempt to create at the
+     * same time.
+     * @return The MemoryTracker singleton.
+     */
+    static MemoryTracker* getInstance(const ALLOCATOR_HOOKS_API& hook_api_);
+
     static void destroyInstance();
 
     void getAllocatorStats(std::map<std::string, size_t> &alloc_stats);
@@ -56,11 +62,18 @@ public:
     size_t getTotalHeapBytes();
 
 private:
-    MemoryTracker();
+    MemoryTracker(const ALLOCATOR_HOOKS_API& hooks_api_);
+
+    // Helper function for construction - connects the tracker
+    // to the memory allocator via alloc_hooks.
+    void connectHooks();
 
     // Function for the stats updater main loop.
     static void statsThreadMainLoop(void* arg);
 
+    static void NewHook(const void* ptr, size_t);
+    static void DeleteHook(const void* ptr);
+
     // Wheter or not we have the ability to accurately track memory allocations
     static bool tracking;
     // Singleton memory tracker and mutex guarding it's creation.
@@ -74,6 +87,10 @@ private:
     std::mutex mutex;
     // Condition variable used to signal shutdown to the stats thread.
     std::condition_variable shutdown_cv;
+
+    // Memory allocator hooks API to use (needed by New / Delete hook
+    // functions)
+    ALLOCATOR_HOOKS_API hooks_api;
 };
 
 #endif  // SRC_MEMORY_TRACKER_H_
index bea1736..a7b11f1 100644 (file)
@@ -47,4 +47,3 @@
 extern void LOG(EXTENSION_LOG_LEVEL severity, const char *fmt, ...) CB_FORMAT_PRINTF(2, 3);
 
 typedef struct engine_allocator_hooks_v1 ALLOCATOR_HOOKS_API;
-extern ALLOCATOR_HOOKS_API *getHooksApi(void);
index 5350481..99492f7 100644 (file)
@@ -18,6 +18,7 @@
 #include "defragmenter_visitor.h"
 
 #include "daemon/alloc_hooks.h"
+#include "programs/engine_testapp/mock_server.h"
 
 #include <gtest/gtest.h>
 #include <iomanip>
@@ -227,7 +228,7 @@ TEST(DefragmenterTest, DISABLED_MappedMemory) {
 #endif
 
     // Instantiate memory tracker (singleton created on-demand).
-    (void)MemoryTracker::getInstance();
+    (void)MemoryTracker::getInstance(*get_mock_server_api()->alloc_hooks);
 
     // Setup object registry. As we do not create a full ep-engine, we
     // use the "initial_tracking" for all memory tracking".
index 3a34a4e..cb6ba19 100644 (file)
@@ -1,6 +1,7 @@
 #include "config.h"
 
 #include "memory_tracker.h"
+#include "mock_hooks_api.h"
 
 #include <thread>
 
@@ -18,9 +19,12 @@ TEST(MemoryTracker, SingletonIsThreadSafe_MB18940) {
     MemoryTracker* instance1;
     MemoryTracker* instance2;
 
+    auto alloc_hooks = *getHooksApi();
+
     // Lambda which returns (via reference param) the result of getInstance().
-    auto get_instance =
-            [](MemoryTracker*& t) { t = MemoryTracker::getInstance(); };
+    auto get_instance = [alloc_hooks](MemoryTracker*& t) {
+        t = MemoryTracker::getInstance(alloc_hooks);
+    };
 
     // Spin up two threads to both call getInstance (and hence attempt to
     // create the singleton).
index 493b290..92f7426 100644 (file)
@@ -17,7 +17,7 @@
 
 #include "config.h"
 
-#include <memcached/allocator_hooks.h>
+#include "mock_hooks_api.h"
 
 /*
  * A mock implementation of the getHooksApi() function (and associated
diff --git a/tests/module_tests/mock_hooks_api.h b/tests/module_tests/mock_hooks_api.h
new file mode 100644 (file)
index 0000000..68bc6f7
--- /dev/null
@@ -0,0 +1,24 @@
+/* -*- Mode: C++; tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+ *     Copyright 2016 Couchbase, Inc
+ *
+ *   Licensed under the Apache License, Version 2.0 (the "License");
+ *   you may not use this file except in compliance with the License.
+ *   You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   Unless required by applicable law or agreed to in writing, software
+ *   distributed under the License is distributed on an "AS IS" BASIS,
+ *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *   See the License for the specific language governing permissions and
+ *   limitations under the License.
+ */
+
+#pragma once
+
+#include "config.h"
+
+#include <memcached/allocator_hooks.h>
+
+ALLOCATOR_HOOKS_API* getHooksApi(void);