Re: [PATCH] Restore mempools functionality for MemBlob

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.

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

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

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

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.

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

Sure.

Thank you,

Alex.
Received on Wed Mar 30 2011 - 16:45:41 MDT