Re: [PATCH] Restore mempools functionality for MemBlob

On 03/30/2011 11:36 PM, Kinkie wrote:
> On Thu, Mar 31, 2011 at 2:27 AM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 03/30/2011 05:58 PM, Kinkie wrote:
>>> + // if pools are not yet ready, make sure that
>>> + // the requested size is not poolable
>>> + if (!MemIsInitialized)
>>> + net_size=1+StrPoolsAttrs[mem_str_pool_count-1].obj_size;
>>> +
>>
>> 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.

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

> +/* explicit initialization state */
> +static bool MemIsInitialized=false;

Consider this description instead:
/// all pools are ready to be used

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.

Should we assert that MemIsInitialized in non-string pools
allocation/deallocation routines?

> + },
> + {
> + "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.

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

>> And more whitespace changes.
> Reverted.

Not all of them though.

Cheers,

Alex.
Received on Thu Mar 31 2011 - 15:18:23 MDT