Re: [PATCH] Restore mempools functionality for MemBlob

On Thu, Mar 31, 2011 at 6:31 PM, Kinkie <gkinkie_at_gmail.com> wrote:
>>>> Depending on the timing of the memAllocString() call and other factors,
>>>> net_size may become 1 after the above. Under other circumstances, it may
>>>> become smaller than it was. Both outcomes would be wrong: net_size can
>>>> only go up.
>>>
>>> Fixed. Thanks.
>>
>> I still do not like that you are relying on StrPoolsAttrs to be in a
>> valid state when memAllocString() is called. Consider rewriting this
>> code so that we do not have to rewrite it yet again when StrPoolsAttrs
>> is no longer a constant POD array. I cannot insist on such a rewrite
>> though.
>
> This is a currently-running assumption, I don't think we're adding much.
> I can use a static const to define this size, no big deal.
>
>> If you do not rewrite, please add the following comment to the "not
>> MemIsInitialized" cases:
>>
>> // relies on StrPoolsAttrs being a static POD constant
>>
>>
>>>      memMeterInc(StrCountMeter);
>>>      memMeterAdd(StrVolumeMeter, *gross_size);
>> ...
>>>      memMeterDec(StrCountMeter);
>>>      memMeterDel(StrVolumeMeter, size);
>>
>> Depending on the timing of the memAlloc/FreeString() calls and other
>> factors, the above meters may forget deallocations and/or allocations
>> because MemMeter is not a POD (in other words, we did not get lucky
>> here). If you do not want to fix this, please add comments:
>> // may forget [de]allocations until MemIsInitialized
>
> I'd do this for now, unless you think it'd be better not to record
> such (de-)allocations at all.
>
>>
>>
>>> +/* explicit initialization state */
>>> +static bool MemIsInitialized=false;
>>
>> Consider this description instead:
>> /// all pools are ready to be used
>
> Done.
>
>> Have you considered just checking whether one of the pools is not NULL
>> instead of maintaining this state explicitly? That check should be
>> wrapped in a MemIsInitialized() inlined function, of course.
>
> That is a possibility, but I guess it has the same drawbacks of
> checking NULL!=strPool[n].pool , hasn't it?
>
>> Should we assert that MemIsInitialized in non-string pools
>> allocation/deallocation routines?
>
> I don't think that they are involved in allocating globals. That's the
> new use case we're trying to cover by overloading memAllocString with
> new functionality.
>
>>> +    },
>>> +    {
>>> +        "Big Strings", MemAllocator::RoundedSize(1024)
>>> +    },
>>> +    {
>>> +        "Huge Strings", MemAllocator::RoundedSize(4096)
>>> +    }  /* other */
>>
>> Since there are many 8KB and 16KB buffers that may be converted to use
>> SBuf, I think we should add a 16KB string pool as well. We can always
>> remove it if post-StringNG stats show that it is not used enough.
>
> I've added a 16kb string. My main concern is that if we have 100
> globals and each needs at least 16kb (may be more if the memory
> allocator uses pages of memory), then it's 2.6Megs of RAM going mostly
> down the drain. Hard to justify on small-footprint systems.
>
>> The old string pool naming scheme does not scale well beyond 4 entries
>> or so. How about naming the new pools "1KB Strings", "4KB Strings", and
>> "16KB Strings"?
>
> Sure. Done.

Whoops, forgot to attach the patch. Here it is of mem.cc alone (as per
Amos's suggestion, I'm splitting the merge in two).

-- 
    /kinkie