Re: [PATCH] Reinstate Mempools useage for MemBlob

On 06/04/11 08:22, Alex Rousskov wrote:
> On 04/05/2011 10:10 AM, Kinkie wrote:
>
>> +/** Given the requested minimum size, return a rounded allocation size
>> * for the backing store.
>> +// TODO: exploit better integration with MemPools, moving much of this there.
>> +// TODO: have MemPools do system page size magic. They don't really belong here
>
> The above comments make no sense given that the new
> MemBlob::calcAllocSize() does nothing. There is no rounding, no "much of
> this", and no "system page size magic" any more.
>
> I suggest removing MemBlob::calcAllocSize() completely and moving any
> still relevant comments to mem.cc.
>
>
>> -#if MEMBLOB_USES_MEM_POOLS
>> - //no mempools for now
>> - // \todo reinstate mempools use
>> memFreeString(capacity,mem);
>
> memFreeString() does not handle deallocation of nil buffers IIRC. It is
> very unlikely but still possible that the mem buffer remains nil in the
> destructor if we hit an exception somewhere along the way to the mem
> allocation call. Let's handle that case gracefully here or in
> memFreeString():
>
> if (mem || capacity)
> memFreeString(capacity,mem);
>
>
> I think this patch can go in after the above polishing touches.

Good point.

Though if it is pushed inside memFreeString (I think it should) that
would make it a part of the mem.cc alterations prior to this
optimization polish.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.6