Re: [PATCH] Restore mempools functionality for MemBlob

On Wed, Mar 30, 2011 at 9:59 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 03/30/2011 01:23 PM, Kinkie wrote:
>
>>>> The original plan was to act exactly as you suggest later on in the
>>>> mail (I'll call it "size-flagging and quoting") for brevity. The
>>>> assertion needs to go anyways, or it'll be hit for objects which are
>>>> small enough to fit in a pool, but not pooled.
>>>
>>> AFAICT, if either of my suggestions is implemented, the assertion can
>>> stay: All early allocations will be too big to trigger it and the
>>> flagged MemBlob would not call this code at all.
>>
>> Well, that's a bit much to ask for.. it would mean a 16kb+ allocation
>> for a 1byte global.
>
> If we only allocate "a few" globals, the overhead would be negligible.
> And if we allocate "a lot", perhaps the proper fix would be to actually
> pool them properly.

Or even better not to pool them at all.

>> Here is an excerpt of the current code for your evaluation.
>> -------------------------
>> void *
>> memAllocString(size_t net_size, size_t * gross_size)
>> {
>>     MemAllocator *pool = NULL;
>>     assert(gross_size);
>>
>>     unsigned int i;
>>     for (i = 0; i < mem_str_pool_count; ++i) {
>>         if (net_size <= StrPoolsAttrs[i].obj_size) {
>>             pool = StrPools[i].pool;
>>             break;
>>         }
>>     }
>>
>>     // we haven't found a pool even though we should have, this
>>     // means that the pools haven't been initialized yet. Increase size
>>     // so that we don't accidentally hit a pool size in memFreeString
>>     if (!pool && StrPoolsAttrs[i].obj_size == net_size )
>>         ++net_size;
>
>
> If you goal is to support early allocations, then the above does not
> make sense to me on many levels:
>
> * If pools are indeed not initialized (and they would not be during some
> static allocations), we should not access StrPoolsAttrs at all! There
> are different initialization stages here, and you have to handle the all
> of them for the code to work reliably.

Why? StrPoolAttrs is statically initialized in the same translation
unit as memAllocString. If memAllocString can be called, it has
already been initlaized. The same cannot be said about MemPools, which
requires MemPools::Init to have completed (it is invoked in main()).
Mem::Init .
The purpose of looking StrPoolAttrs up is to find the index to be
looked up in StrPools to find the actual allocator. StrPools is also
global and it contains a pointer to the actual allocator, to be
initialized at the end of Mem::Init.
Which means that all data accesses should be ok as long as we don't go
past the end of arrays, but this should be guarded by no loops
exceeding mem_str_pool_count. There are thus three possible outcomes:
- we can't find the pool by searching the attributes in StrPoolAttrs
- we can find an appropriate pool but can't find the allocator because
its pointer is zero on account of it being an yet-unassigned global
- we can find the pool
The only intermediate stages would be if we were to allocate a String
during the execution of Mem::Init, but even in presence of
multithreading, Mem::Init is invoked before any threads are spawned.

Maybe I'm tired, but I can find no obvious holes. I agree it's
butt-ugly and full of unexpressed assumptions. We also agreed that
this is an interim implementation, and that MemBlob will likely
warrant writing a specific pooling system, which would eventually
absorb memAllocString and memAllocBuf.

> * If we did not find the pool, "i" may point beyond the StrPoolsAttrs array.

Right. Oops.

> * We could find no pool because there is no pool large enough for our
> net_size and not because pools have not been initialized yet. This can
> be fixed by starting the comment with "If " and rewarding the rest of
> the comment, I guess.
>
> Etc.

Yes.

>
>
>> void
>> memFreeString(size_t size, void *buf)
>> {
>>     MemAllocator *pool = NULL;
>>     assert(buf);
>>
>>     for (unsigned int i = 0; i < mem_str_pool_count; ++i) {
>>         if (size == StrPoolsAttrs[i].obj_size) {
>>           pool = StrPools[i].pool;
>>             break;
>>         }
>>     }
>>
>>     memMeterDec(StrCountMeter);
>>     memMeterDel(StrVolumeMeter, size);
>>     pool ? pool->freeOne(buf) : xfree(buf);
>> }
>
> And to add insult to injury, we now completely lost the check that
> pooled strings are not xfreed due to wrong size being given to
> memFreeString().

We don't have much of a defense against that anyways. One just needs
to hit the size of the wrong pool

>> This code is build-tested and passes the test-suite.
>
> Which says a lot about the quality of that test-suite...

It did catch a few issues with earlier versions of the code. It can be
improved, of course.

In any case, I failed to convince you of the viability of this
approach. As promised, I'll then implement your suggestion of
assigning max(pool_size)+1 to early allocations; I'll also add an
explicit initialization state to be set by Mem::Init so that we can
have an explicit intialization check.

Thanks!

-- 
    /kinkie