[squid-dev] [PATCH] Bug 4438 - string pool refactoring
Amos Jeffries
squid3 at treenet.co.nz
Tue Mar 29 16:15:26 UTC 2016
On 26/03/2016 3:28 a.m., Alex Rousskov wrote:
> On 03/25/2016 07:08 AM, Amos Jeffries wrote:
>> This was audited off-list, and a slightly more polished version applied
>> to trunk as rev.14604
>
> I have not seen the off-list audit, but the committed version is buggy IMHO:
>
<http://pixy.kinkie.it/~kinkie/irc-logs/bot/index.php?d=2016-03-23>
"
[20:07:04]
yadi
what I mean is the pools are MemPool class instances. if they get
destructed on exit() while some other class is still not-yet destructed
and using pooled memory we have trouble.
[21:24:58]
rousskov
yadi, I know that you have meant that. What I am saying is that MemPool
cannot be destructed until there are no allocations alive.
"
I may still be misunderstanding you. I took that as meaning we had it right.
>
>> MemPools &
>> MemPools::GetInstance()
>> {
>> /* Must use this idiom, as we can be double-initialised
>> * if we are called during static initialisations.
>> */
>> - if (!Instance)
>> - Instance = new MemPools;
>> - return *Instance;
>> + static MemPools Instance;
>> + return Instance;
>> }
>
>
> This change solves the initialization order bug but introduces the
> destruction order bug: The Instance object may be gone before its last
> use. Please fix and check for similar bugs in the new code.
>
Fixed by dynamically allocating Instance and leaking it as agreed on IRC
today.
The only other object which might share this problem is GetPool(t). Its
object is POD though. The complex part of that is dynamically allocated
already.
>
>> +static MemAllocator *&
>> +GetPool(size_t type)
>> +{
>> + static MemAllocator *pools[MEM_MAX];
>> + static bool initialized = false;
>> +
>> + if (!initialized) {
>> + memset(pools, '\0', sizeof(pools));
>> + initialized = true;
>> + }
>> +
>> + return pools[type];
>> +}
>
>> /* find appropriate pool and use it (pools always init buffer with 0s) */
>> void *
>> memAllocate(mem_type type)
>> {
>> - assert(MemPools[type]);
>> - return MemPools[type]->alloc();
>> + assert(GetPool(type));
>> + return GetPool(type)->alloc();
>> }
>
> AFAICT, this combination will assert if memAllocate() (or any similar
> external caller) is called "too early". Since memAllocate() is not
> static, you cannot control when it is called. Please fix so that
> GetPool() always returns a usable pool. Returning a _reference_ to that
> pool instead of a never-nil pointer would be appropriate.
>
That is intentional. This function is only used by 'old API' pools which
must not be used prior to Mem::Init() being called from mainInitialize().
I've made the situation a little bit safer by calling Mem::Init() in the
initialization branch of GetPool(). But the risk still exists for pools
which are not initialized by Mem::init(). Having an assert for that case
is still best as it highlights that the code being changed is broken
when the dev runs the change.
We can't return a reference here since the initializer functions make
use of GetPool(t) to locate the array pointer to be set.
>
>> +static MemAllocator *&
>> +GetStrPool(size_t type)
>
> Wrong return type AFAICT: This function always returns a usable pool so
> it should return a reference and the callers should be adjusted to avoid
> treating non-existent nil string pools specially. The latter adjustment
> may expose other necessary changes.
Done.
Updated patch attached.
Thanks
Amos
-------------- next part --------------
=== modified file 'src/mem/AllocatorProxy.cc'
--- src/mem/AllocatorProxy.cc 2016-01-01 00:12:18 +0000
+++ src/mem/AllocatorProxy.cc 2016-03-28 22:20:40 +0000
@@ -41,7 +41,7 @@
if (!theAllocator)
return 0;
else
- return memPoolInUseCount(theAllocator);
+ return theAllocator->inUseCount();
}
void
=== modified file 'src/mem/Pool.cc'
--- src/mem/Pool.cc 2016-03-23 11:36:59 +0000
+++ src/mem/Pool.cc 2016-03-28 22:20:29 +0000
@@ -32,8 +32,11 @@
/* Must use this idiom, as we can be double-initialised
* if we are called during static initialisations.
*/
- static MemPools Instance;
- return Instance;
+ static MemPools *Instance = nullptr;
+ if (!Instance) {
+ Instance = new MemPools;
+ }
+ return *Instance;
}
MemPoolIterator *
@@ -298,12 +301,6 @@
}
int
-memPoolInUseCount(MemAllocator * pool)
-{
- return pool->inUseCount();
-}
-
-int
memPoolsTotalAllocated(void)
{
MemPoolGlobalStats stats;
=== modified file 'src/mem/Pool.h'
--- src/mem/Pool.h 2016-03-23 16:29:47 +0000
+++ src/mem/Pool.h 2016-03-28 22:21:14 +0000
@@ -356,8 +356,6 @@
extern int memPoolGetGlobalStats(MemPoolGlobalStats * stats);
/// \ingroup MemPoolsAPI
-extern int memPoolInUseCount(MemAllocator *);
-/// \ingroup MemPoolsAPI
extern int memPoolsTotalAllocated(void);
#endif /* _MEM_POOL_H_ */
=== modified file 'src/mem/old_api.cc'
--- src/mem/old_api.cc 2016-03-25 12:55:30 +0000
+++ src/mem/old_api.cc 2016-03-29 16:11:08 +0000
@@ -69,12 +69,15 @@
if (!initialized) {
memset(pools, '\0', sizeof(pools));
initialized = true;
+ // Mem::Init() makes use of GetPool(type) to initialize
+ // the actual pools. So must come after the flag is true
+ Mem::Init();
}
return pools[type];
}
-static MemAllocator *&
+static MemAllocator &
GetStrPool(size_t type)
{
static MemAllocator *strPools[mem_str_pool_count];
@@ -107,7 +110,7 @@
initialized = true;
}
- return strPools[type];
+ return *strPools[type];
}
/* Find the best fit string pool type */
@@ -116,13 +119,11 @@
{
mem_type type = MEM_NONE;
for (unsigned int i = 0; i < mem_str_pool_count; ++i) {
- auto pool = GetStrPool(i);
- if (!pool)
- continue;
- if (fuzzy && net_size < pool->objectSize()) {
+ auto &pool = GetStrPool(i);
+ if (fuzzy && net_size < pool.objectSize()) {
type = static_cast<mem_type>(i);
break;
- } else if (net_size == pool->objectSize()) {
+ } else if (net_size == pool.objectSize()) {
type = static_cast<mem_type>(i);
break;
}
@@ -142,13 +143,13 @@
/* table body */
for (i = 0; i < mem_str_pool_count; ++i) {
- const MemAllocator *pool = GetStrPool(i);
- const auto plevel = pool->getMeter().inuse.currentLevel();
- stream << std::setw(20) << std::left << pool->objectType();
+ const auto &pool = GetStrPool(i);
+ const auto plevel = pool.getMeter().inuse.currentLevel();
+ stream << std::setw(20) << std::left << pool.objectType();
stream << std::right << "\t " << xpercentInt(plevel, StrCountMeter.currentLevel());
- stream << "\t " << xpercentInt(plevel * pool->objectSize(), StrVolumeMeter.currentLevel()) << "\n";
+ stream << "\t " << xpercentInt(plevel * pool.objectSize(), StrVolumeMeter.currentLevel()) << "\n";
pooled_count += plevel;
- pooled_volume += plevel * pool->objectSize();
+ pooled_volume += plevel * pool.objectSize();
}
/* malloc strings */
@@ -233,18 +234,22 @@
void *
memAllocString(size_t net_size, size_t * gross_size)
{
- MemAllocator *pool = NULL;
assert(gross_size);
auto type = memFindStringSizeType(net_size, true);
- if (type != MEM_NONE)
- pool = GetStrPool(type);
+ if (type != MEM_NONE) {
+ auto &pool = GetStrPool(type);
+ *gross_size = pool.objectSize();
+ assert(*gross_size >= net_size);
+ ++StrCountMeter;
+ StrVolumeMeter += *gross_size;
+ return pool.alloc();
+ }
- *gross_size = pool ? pool->objectSize() : net_size;
- assert(*gross_size >= net_size);
+ *gross_size = net_size;
++StrCountMeter;
StrVolumeMeter += *gross_size;
- return pool ? pool->alloc() : xcalloc(1, net_size);
+ return xcalloc(1, net_size);
}
size_t
@@ -253,7 +258,7 @@
size_t result = 0;
for (int counter = 0; counter < mem_str_pool_count; ++counter)
- result += memPoolInUseCount(GetStrPool(counter));
+ result += GetStrPool(counter).inUseCount();
return result;
}
@@ -262,16 +267,16 @@
void
memFreeString(size_t size, void *buf)
{
- MemAllocator *pool = NULL;
assert(buf);
auto type = memFindStringSizeType(size, false);
if (type != MEM_NONE)
- pool = GetStrPool(type);
+ GetStrPool(type).freeOne(buf);
+ else
+ xfree(buf);
--StrCountMeter;
StrVolumeMeter -= size;
- pool ? pool->freeOne(buf) : xfree(buf);
}
/* Find the best fit MEM_X_BUF type */
@@ -509,7 +514,7 @@
int
memInUse(mem_type type)
{
- return memPoolInUseCount(GetPool(type));
+ return GetPool(type)->inUseCount();
}
/* ick */
=== modified file 'src/tests/stub_libmem.cc'
--- src/tests/stub_libmem.cc 2016-03-23 16:29:47 +0000
+++ src/tests/stub_libmem.cc 2016-03-28 22:21:25 +0000
@@ -105,6 +105,5 @@
MemImplementingAllocator * memPoolIterateNext(MemPoolIterator * iter) STUB_RETVAL(NULL)
void memPoolIterateDone(MemPoolIterator ** iter) STUB
int memPoolGetGlobalStats(MemPoolGlobalStats * stats) STUB_RETVAL(0)
-int memPoolInUseCount(MemAllocator *) STUB_RETVAL(0)
int memPoolsTotalAllocated(void) STUB_RETVAL(0)
More information about the squid-dev
mailing list