MemPools: remove zero on allocate (calloc) from some pool types

As we are closing defects identified by Coverity and improving
constructors everywhere we are creating a minor anti-pattern in
MemPool'ed objects with calloc() in the pool initializing the memory
then constructors re-initializing it in a better way for that object.

MemPools contains a doZeroOnPush flag to optimize performance by removing
use of memset() as chunks are added back into the pool. However, on
closer inspection it is clear that the following pop() process to re-use
those chunks is never performing memset() anyway. As such I believe that
there is no special need to use calloc() on these particular object types
in the first place.

Update MemPools to use malloc() instead of calloc() on all types with
doZeroOnPush set. This should increase performance a little, and allows
us to remove the anti-pattern by setting doZeroOnPush for more objects
as we can verify they are correctly initialized by their constructors.

diff --git a/include/MemPool.h b/include/MemPool.h
index ac1ce16de41d4608deef4e65edf5f9ee94f949be..7e3716c9d6ac7b1fa48890726bd84a3c0a3ed4af 100644 (file)
--- a/include/MemPool.h
+++ b/include/MemPool.h
@@ -204,7 +204,7 @@ public:
     virtual char const *objectType() const;
     virtual size_t objectSize() const = 0;
     virtual int getInUseCount() = 0;
-    void zeroOnPush(bool doIt);
+    void zeroBlocks(bool doIt) {doZero = doIt;}
     int inUseCount();
 
     /**
@@ -226,7 +226,13 @@ public:
     static size_t RoundedSize(size_t minSize);
 
 protected:
-    bool doZeroOnPush;
+    /** Whether to zero memory on initial allocation and on return to the pool.
+     *
+     * We do this on some pools because many object constructors are/were incomplete
+     * and we are afraid some code may use the object after free.
+     * These probems are becoming less common, so when possible set this to false.
+     */
+    bool doZero;
 
 private:
     const char *label;

diff --git a/lib/MemPool.cc b/lib/MemPool.cc
index c5b59cbe82eb8f97700e06775efc2c85e3f40b83..82ee559c4d2e1654e1169b2dfcc87d674025d259 100644 (file)
--- a/lib/MemPool.cc
+++ b/lib/MemPool.cc
@@ -324,7 +324,7 @@ memPoolGetGlobalStats(MemPoolGlobalStats * stats)
     return pools_inuse;
 }
 
-MemAllocator::MemAllocator(char const *aLabel) : doZeroOnPush(true), label(aLabel)
+MemAllocator::MemAllocator(char const *aLabel) : doZero(true), label(aLabel)
 {
 }
 
@@ -445,12 +445,6 @@ MemImplementingAllocator::~MemImplementingAllocator()
     --MemPools::GetInstance().poolCount;
 }
 
-void
-MemAllocator::zeroOnPush(bool doIt)
-{
-    doZeroOnPush = doIt;
-}
-
 MemPoolMeter const &
 MemImplementingAllocator::getMeter() const
 {

diff --git a/lib/MemPoolChunked.cc b/lib/MemPoolChunked.cc
index ac91ad6fd4cc26bfcbcb66645330152ff3c65e23..7a12e6e662afc2aa5ea8e6979c680b41dba0cca5 100644 (file)
--- a/lib/MemPoolChunked.cc
+++ b/lib/MemPoolChunked.cc
@@ -141,7 +141,11 @@ MemChunk::MemChunk(MemPoolChunked *aPool)
     next = NULL;
     pool = aPool;
 
-    objCache = xcalloc(1, pool->chunk_size);
+    if (pool->doZero)
+        objCache = xcalloc(1, pool->chunk_size);
+    else
+        objCache = xmalloc(pool->chunk_size);
+
     freeList = objCache;
     void **Free = (void **)freeList;
 
@@ -196,7 +200,7 @@ MemPoolChunked::push(void *obj)
      * not really need to be cleared.. There was a condition based on
      * the object size here, but such condition is not safe.
      */
-    if (doZeroOnPush)
+    if (doZero)
         memset(obj, 0, obj_size);
     Free = (void **)obj;
     *Free = freeCache;

diff --git a/lib/MemPoolMalloc.cc b/lib/MemPoolMalloc.cc
index 068067d34bc978adc7a2876a78c513a9a34a2481..06f18516e353d922e6bffbdf111bf99bffd8be15 100644 (file)
--- a/lib/MemPoolMalloc.cc
+++ b/lib/MemPoolMalloc.cc
@@ -56,7 +56,10 @@ MemPoolMalloc::allocate()
         memMeterDec(meter.idle);
         ++saved_calls;
     } else {
-        obj = xcalloc(1, obj_size);
+        if (doZero)
+            obj = xcalloc(1, obj_size);
+        else
+            obj = xmalloc(obj_size);
         memMeterInc(meter.alloc);
     }
     memMeterInc(meter.inuse);
@@ -71,7 +74,7 @@ MemPoolMalloc::deallocate(void *obj, bool aggressive)
         xfree(obj);
         memMeterDec(meter.alloc);
     } else {
-        if (doZeroOnPush)
+        if (doZero)
             memset(obj, 0, obj_size);
         memMeterInc(meter.idle);
         freelist.push_back(obj);

diff --git a/src/Mem.h b/src/Mem.h
index 82c047cd6452d99b04e09ae00dcd4556cce4858b..343e7719b57c287c300622618edefefc592da199 100644 (file)
--- a/src/Mem.h
+++ b/src/Mem.h
@@ -75,7 +75,7 @@ void memFreeString(size_t size, void *);
 void memFreeBuf(size_t size, void *);
 FREE *memFreeBufFunc(size_t size);
 int memInUse(mem_type);
-void memDataInit(mem_type, const char *, size_t, int, bool zeroOnPush = true);
+void memDataInit(mem_type, const char *, size_t, int, bool doZero = true);
 void memCheckInit(void);
 void memConfigure(void);
 

diff --git a/src/mem.cc b/src/mem.cc
index 678a49f3278cf0db6ce9b4b32c8293c9681ba593..cb1156c5ddd6bfc080dfb98ab795189a613cecfa 100644 (file)
--- a/src/mem.cc
+++ b/src/mem.cc
@@ -201,7 +201,7 @@ Mem::Stats(StoreEntry * sentry)
  * Relies on Mem::Init() having been called beforehand.
  */
 void
-memDataInit(mem_type type, const char *name, size_t size, int max_pages_notused, bool zeroOnPush)
+memDataInit(mem_type type, const char *name, size_t size, int max_pages_notused, bool doZero)
 {
     assert(name && size);
 
@@ -209,7 +209,7 @@ memDataInit(mem_type type, const char *name, size_t size, int max_pages_notused,
         return;
 
     MemPools[type] = memPoolCreate(name, size);
-    MemPools[type]->zeroOnPush(zeroOnPush);
+    MemPools[type]->zeroBlocks(doZero);
 }
 
 /* find appropriate pool and use it (pools always init buffer with 0s) */
@@ -477,7 +477,7 @@ Mem::Init(void)
     /** Lastly init the string pools. */
     for (i = 0; i < mem_str_pool_count; ++i) {
         StrPools[i].pool = memPoolCreate(StrPoolsAttrs[i].name, StrPoolsAttrs[i].obj_size);
-        StrPools[i].pool->zeroOnPush(false);
+        StrPools[i].pool->zeroBlocks(false);
 
         if (StrPools[i].pool->objectSize() != StrPoolsAttrs[i].obj_size)
             debugs(13, DBG_IMPORTANT, "Notice: " << StrPoolsAttrs[i].name << " is " << StrPools[i].pool->objectSize() << " bytes instead of requested " << StrPoolsAttrs[i].obj_size << " bytes");