Re: [PATCH] Restore mempools functionality for MemBlob

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.
This path would be hit for MemBlobs which are allcoated before
MemPools are initialized (e.g. globals). 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.

>> +// TODO: have MemPools do system page size magic. They don't really belong here
>
> but the corresponding patched code does not refer to the "system page
> size" anymore. Delete this TODO? More on this code below.

Expunged.

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

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

> I may be missing something, but it feels like we just need to take care
> of the case where memory allocation failed (due to an exception or
> whatnot), leaving capacity and buf zero in the destructor:
>
> MemBlob::~MemBlob()
> {
>    if (capacity || mem) // memFreeString detects any inconsistencies
>        memFreeString(capacity,mem);
>    ...
> }

Nice touch. Added.

>> -#if HAVE_OSTREAM
>> -#include <ostream>
>> +#if HAVE_IOSFWD
>> +#include <iosfwd>
>>  #endif
>
> Is this change related to the "increased flexibility"? If not, please
> remove. If the change is needed for other reasons, it is OK to commit it
> separately, of course.

You're right, it's not.
I guess this can go in as a build-tested, unreviewed, commit to trunk,
as it's mostly cosmetic.

>> @@ -70,7 +70,6 @@
>>  }
>>
>>  StrPoolsAttrs[mem_str_pool_count] = {
>> -
>>      {
>>          "Short Strings", MemAllocator::RoundedSize(36),
>>      },                               /* to fit rfc1123 and similar */
> ...
>> -    int i;
>> +     int i;
>
> Please avoid whitespace changes.
>
>
> Please include proposed commit message as the patch preamble (or in the
> email text).
>
> Please use 20-line diffs (-U 20) for patches if possible. Seeing more
> context often makes it easier to review the changes.

Ok.

-- 
    /kinkie