Re: [PATCH] Enable string mempools to work correctly during initialization phase

On Tue, Apr 5, 2011 at 10:10 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 04/05/2011 07:07 AM, Kinkie wrote:
>
>> Extends string mempools so that they can work before the pools
>> subsystem has been fully initialized, as it happens for instance for
>> global variables. It does so by explicitly tracking the initialization
>> state and by expanding early-allocated strings' sizes in order to mark
>> them as non-pooled.
>
> Consider this as a more precise description:

Ok, thanks!

>
> ----------
> Makes string mempools work before Mem::Init() was called, as may happen
> during global variable initialization or early main.cc processing. If
> needed, strings allocated before the Mem::Init() call are given an extra
> buffer space to make sure the allocated buffer size will not match any
> string pool size during deallocation.
>
> Shortcomings: We now waste RAM on buffer increase for early allocated
> strings unless they are already bigger than the maximum supported string
> pool size. Statistics for early allocations is still broken. Non-string
> mempools still do not support early allocations.
> ----------
>
>
> * If you can use SizeOfStringBeforePoolsAreInitialized when inializing
> the last (biggest) StrPoolsAttrs member, please do so. If you cannot,
> add a comment to that member, pointing to the link with
> SizeOfStringBeforePoolsAreInitialized that must be kept in sync.

Implemented the first suggestion.

>> +// must be at least one byte bigger than the biggest string size
>
> ... than the biggest string pool size
> (strings themselves may be bigger than that)

Done.

>> +static const size_t SizeOfStringBeforePoolsAreInitialized = 513;
>
> Not all early-allocated strings will be 513 bytes long. You should also
> tie this with MemIsInitialized name. Consider this name instead:
> SmallestStringBeforeMemIsInitialized

Done.

> * Please add spaces around "=" in new assignments unless
> SourceFormatting scripts will do that for you.

Done.

> I have no serious objections to this patch, with or without the above
> changes. There is no need to repost unless others need more changes. I
> think it would be better to avoid MemIsInitialized and the dependency on
> StrPoolsAttrs being a constant POD, but we can do that later.

Agreed. I have a few ideas, but not before StringNG is done.

I will also merge in comments from the MemBlob patch, as recommended there.

-- 
    /kinkie