Re: [PATCH] Restore mempools functionality for MemBlob

On Wed, Mar 30, 2011 at 8:48 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 03/30/2011 12:13 PM, Kinkie wrote:
>> On Wed, Mar 30, 2011 at 6:45 PM, Alex Rousskov
>> <rousskov_at_measurement-factory.com> wrote:
>>> On 03/30/2011 10:01 AM, Kinkie wrote:
>>>> On Wed, Mar 30, 2011 at 5:46 PM, Alex Rousskov
>>>> <rousskov_at_measurement-factory.com> wrote:
>>>>> On 03/30/2011 04:34 AM, Kinkie wrote:
>>>>>
>>>>>>   the attached patch increases the flexibility of mempools in
>>>>>> supporting strings, and as a consequence enables MemBlob to use
>>>>>> MemPools.
>>>>>
>>>>> Could you please explain what the changes really are? "Increases
>>>>> flexibility of mempools" does not tell me much about the changes and
>>>>> does not allow me to compare the changes with the intended consequences.
>>>>> This is especially important when you are changing complex core pieces
>>>>> such as memory pools.
>>>>
>>>> Sure (a revised patch is coming up to address Amos' objections).
>>>> memFreeString used to be strict in enforcing adherence to pool sizes:
>>>> it would assert at shutdown if used on a blob which was smaller than
>>>> the biggest available pool size and didn't match exactly a pool size.
>>>
>>> I believe that assert was correct for the supported string pool usage.
>>>
>>>> This path would be hit for MemBlobs which are allcoated before
>>>> MemPools are initialized (e.g. globals).
>>>
>>> It would also get hit when somebody frees a string that was not pooled
>>> or pooled with a different size than the freeing code supplied,
>>> preventing mysterious coredumps.
>>>
>>>
>>>> The new code doesn't enforce
>>>> this limit anymore: pools can handle the case of nonpool-ed blobs
>>>> being pool-freed, and xfree is called for all other cases anyways.
>>>
>>> The proposed code does not check that the right pool is used anymore.
>>> This is a negative side-effect of your changes. Is it worth the
>>> benefits? Depends on what the alternatives are.
>>
>> Pools should be able to handle this case in a robust manner; in fact
>> this is a code-path which can be hit if a MemBlob allocates a chunk of
>> memory which has the same size of a pool before pools are initialized.
>
> Current code does not support early allocations (pre-confgiguration
> pooling). It is a good idea to improve that, of course, but I do not
> think that requires removing the existing checks. If you propose to
> remove them, let's discuss whether that is the best way forward.
>
>> The case at hand is supporting globals and mempools, and selecting the
>> right destructor if mempools are not available,
>
> Sure.
>
>> without using flags in MemBlob.
>
> That part is not a use case. That is one possible solution.

Yes, sorry for mixing things. Maybe a better wording could be "with as
small a memory overhead as possible"

>> 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.
My suggestion would be to use any size which is not a pool, and then
change the assert to an equality test. It would mean one byte of
overhead for each global.

>> During discussions on IRC it was suggested that it may be not needed,
>> as pools should be able to handle such corner (we may call it "error")
>> cases.
>
> The assertion was there to check for coding bugs. Removing it without
> replacement will expose us to those bugs. Removing it may also help
> cover more use cases, but that does not necessarily mean that the
> assertion "is not needed".

Agreed.
It does cover a rather narrow error case though: for instance it
doesn't cover the case where a String is mis-freed using a valid size
(but for a different pool).

>>> Please consider leaving the current safety checks in place and just
>>> allocating 2*M or M+1 size strings when pools are not initialized, where
>>> M is the maximum supported string pool size. This will pass all
>>> memFreeString checks and automatically use xfree for those "early
>>> allocations", right?
>>
>> Yes. That was the original plan. I will implement it. Just making sure
>> that the size of the allocated blob is an odd number will do.
>
> Using odd numbers will make the current check less effective -- it will
> have to be bypassed whenever the submitted size is odd (which could be
> due to the "needed" size given to memFreeString instead of "allocated").
> Given that StringNG is about to change a lot of memory-related code, I
> think it would be better to leave the check as is and sacrifice a few
> KBs of RAM instead.

If I haven't managed to convince you with this mail, I'll do it the
over-sizing you suggest.
In time, it's quite likely that MemBlob will have its own pool
allocator anyways.

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;

    *gross_size = pool ? StrPoolsAttrs[i].obj_size : net_size;
    assert(*gross_size >= net_size);
    memMeterInc(StrCountMeter);
    memMeterAdd(StrVolumeMeter, *gross_size);
    return pool ? pool->alloc() : xcalloc(1, net_size);
}

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);
}

------------------------------------------------------------

This code is build-tested and passes the test-suite.

>>> In the unlikely event of the above approach creating too much RAM waste
>>> due to huge volume of pre-pooled allocations, we can instead add a flag
>>> to MemBlob so that it knows to use xfree instead of memFreeString for
>>> "early allocations".
>>
>> This would be worse. Size-implied flagging wastes at most one word per
>> global object. A flag to MemBlob means most likely one extra word per
>> MemBlob (global or not), which is much more.
>
> I am not sure one word per MemBlob is more waste than a few KBs per
> early allocation. It depends on how much we end up allocating early.
> However, avoiding flags may be better because it allows non-MemBlob
> users to allocate early.

My proposal wastes one byte per early allocation.
I agree with your second argument.

Thanks.

-- 
    /kinkie