Re: [PATCH] Restore mempools functionality for MemBlob

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

>>> And more whitespace changes.
>> Reverted.
>
> Not all of them though.

I've shuffled a couple of blank lines around, I think. IMVHO it makes
things more readable, if you want I can revert them as well.

Thanks.

-- 
    /kinkie