From: Amos Jeffries Date: Mon, 14 Jan 2013 05:01:04 +0000 (-0700) Subject: MemPools: remove zero on allocate (calloc) from some pool types X-Git-Tag: SQUID_3_4_0_1~380 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3b08e399e164a37e16ea36a560d12fd7f44b48fe;p=thirdparty%2Fsquid.git 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 ac1ce16de4..7e3716c9d6 100644 --- 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 c5b59cbe82..82ee559c4d 100644 --- 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 ac91ad6fd4..7a12e6e662 100644 --- 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 068067d34b..06f18516e3 100644 --- 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 82c047cd64..343e7719b5 100644 --- 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 678a49f327..cb1156c5dd 100644 --- 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");