On 13/01/2013 6:51 a.m., Alex Rousskov wrote:
On 01/11/2013 12:34 AM, Amos Jeffries wrote:
MemPools contains a doZeroOnPush flag to optimize performance by
removing use of memset() as chunks are added back into the pool.
yes, although to avoid misunderstanding I would phrase it differently:
"If doZeroOnPush is true, the pool zeros chunks 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.
Here, I assume you are talking about pools which have zeroOnPush set to
false.
As
such I believe that there is no special need to use calloc() on these
particular object types in the first place.
The attached patch updates MemPools to use malloc() instead of calloc()
on all types with doZeroOnPush set.
Yes, if "set" means "set to false" :-)
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.
Again, I assume "setting doZeroOnPush" means "setting doZeroOnPush to
false". In your commit message, please use a less misleading terminology.
Yes.
Please rename doZeroOnPush and friends to doZero or at least document
that member to indicate that it affects both allocated and pushed data now.
While you are documenting, it may be a good idea to explain that we zero
on push because we are afraid of broken code using stale/dirty data
after the memory was returned to the pool. If we were not afraid of
that, we would zero on allocate() instead!
Will do.
I think we do not need to have that worry anymore with the C++ compiler
and Coverity use-after-free checks. Do you agree?
Updated patch attached for re-review.
Amos
=== modified file 'include/MemPool.h'
--- include/MemPool.h 2012-08-28 13:00:30 +0000
+++ include/MemPool.h 2013-01-12 22:12:27 +0000
@@ -204,7 +204,7 @@
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 @@
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;
=== modified file 'lib/MemPool.cc'
--- lib/MemPool.cc 2012-09-01 14:38:36 +0000
+++ lib/MemPool.cc 2013-01-12 22:13:05 +0000
@@ -324,7 +324,7 @@
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 @@
--MemPools::GetInstance().poolCount;
}
-void
-MemAllocator::zeroOnPush(bool doIt)
-{
- doZeroOnPush = doIt;
-}
-
MemPoolMeter const &
MemImplementingAllocator::getMeter() const
{
=== modified file 'lib/MemPoolChunked.cc'
--- lib/MemPoolChunked.cc 2012-09-01 14:38:36 +0000
+++ lib/MemPoolChunked.cc 2013-01-12 22:01:58 +0000
@@ -141,7 +141,11 @@
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 @@
* 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;
=== modified file 'lib/MemPoolMalloc.cc'
--- lib/MemPoolMalloc.cc 2012-09-01 14:38:36 +0000
+++ lib/MemPoolMalloc.cc 2013-01-12 22:08:41 +0000
@@ -56,7 +56,10 @@
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 @@
xfree(obj);
memMeterDec(meter.alloc);
} else {
- if (doZeroOnPush)
+ if (doZero)
memset(obj, 0, obj_size);
memMeterInc(meter.idle);
freelist.push_back(obj);
=== modified file 'src/Mem.h'
--- src/Mem.h 2012-09-21 14:57:30 +0000
+++ src/Mem.h 2013-01-12 22:38:36 +0000
@@ -75,7 +75,7 @@
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);
=== modified file 'src/mem.cc'
--- src/mem.cc 2012-10-04 09:14:06 +0000
+++ src/mem.cc 2013-01-12 22:36:35 +0000
@@ -201,7 +201,7 @@
* 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 @@
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 @@
/** 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");
