Re: [PATCH] Restore mempools functionality for MemBlob

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.

The case at hand is supporting globals and mempools, and selecting the
right destructor if mempools are not available, without using flags in
MemBlob.
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.
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. But size-quoting can easily be reinstated.

>>>> -    assert(size && buf);
>>>> +    assert(buf);
>>>>
>>>>      for (i = 0; i < mem_str_pool_count; i++) {
>>>> -        if (size <= StrPoolsAttrs[i].obj_size) {
>>>> -            assert(size == StrPoolsAttrs[i].obj_size);
>>>> -            pool = StrPools[i].pool;
>>>> +        if (size > StrPoolsAttrs[i].obj_size)
>>>> +             break;
>>>> +        if (size == StrPoolsAttrs[i].obj_size) {
>>>> +                     pool = StrPools[i].pool;
>>>>              break;
>>>>          }
>>>
>>> The above change does not make sense to me. My understanding is that the
>>> undocumented memFreeString() function expects the actual allocated size
>>> as the parameter. It relies on that expectation to find the right pool.
>>> Thus, the removed code was correct. Moreover, we do supply the right
>>> size value in the destructor.
>>
>> This code is broken (see Amos' comment).
>
> Yes, I saw it, but my concern is not just about the bug you added while
> changing memFreeString (I am sure you can fix that bug) but about the
> apparent (albeit currently undocumented) intent to remove "we are using
> the right pool" controls from that function.
>
> 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.

> 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.

>>>> >>  MemBlob::calcAllocSize(const size_type sz) const
>>>> >>  {
>>>> >>      if (sz <= 36) return 36;
>>>> >>      if (sz <= 128) return 128;
>>>> >>      if (sz <= 512) return 512;
>>>> >>      if (sz <= 4096) return RoundTo(sz, 512);
>>>> >> -    // XXX: recover squidSystemPageSize functionality. It's easy for
>>>> >> -    //      the main squid, harder for tests
>>>> >> -#if 0
>>>> >> -    return RoundTo(sz, squidSystemPageSize);
>>>> >> -#else
>>>> >>      return RoundTo(sz, 4096);
>>>> >> -#endif
>>>> >>  }
>>> >
>>> > Why do we need the above if memAllocString() already has similar logic
>>> > and can overwrite our decision anyway?
>> Agreed. At the same time memAllocString and memAllocBuf have
>> significant functional overlaps, and I'd like to see them merged. This
>> is however out of scope for StringNG.
>> I'll no-op and #ifdef the code as a reminder for that project.
>
> I agree that we should eventually merge "string" and "buf" pools, but
> that out-of-scope change is not critical.

Yes. That's why I'm not bringing that in scope to StringNg.

> Since MemBlobs are used for more than just short strings, I think we
> should add a "huge" 8KB or 16KB string pool now, as a part of this patch
> or a separate "optimization" commit.

Ok.

Next steps:
- implement size-quoting in memAllocString
- implement a couple more string sizes ("big" 4k and "huge" 16k?)
- test and resubmit

Thanks.

-- 
    /kinkie