Re: [PATCH] Restore mempools functionality for MemBlob

On 03/30/2011 12:13 PM, Kinkie wrote:
> On Wed, Mar 30, 2011 at 6:45 PM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 03/30/2011 10:01 AM, Kinkie wrote:
>>> On Wed, Mar 30, 2011 at 5:46 PM, Alex Rousskov
>>> <rousskov_at_measurement-factory.com> wrote:
>>>> On 03/30/2011 04:34 AM, Kinkie wrote:
>>>>
>>>>> the attached patch increases the flexibility of mempools in
>>>>> supporting strings, and as a consequence enables MemBlob to use
>>>>> MemPools.
>>>>
>>>> Could you please explain what the changes really are? "Increases
>>>> flexibility of mempools" does not tell me much about the changes and
>>>> does not allow me to compare the changes with the intended consequences.
>>>> This is especially important when you are changing complex core pieces
>>>> such as memory pools.
>>>
>>> Sure (a revised patch is coming up to address Amos' objections).
>>> memFreeString used to be strict in enforcing adherence to pool sizes:
>>> it would assert at shutdown if used on a blob which was smaller than
>>> the biggest available pool size and didn't match exactly a pool size.
>>
>> I believe that assert was correct for the supported string pool usage.
>>
>>> This path would be hit for MemBlobs which are allcoated before
>>> MemPools are initialized (e.g. globals).
>>
>> It would also get hit when somebody frees a string that was not pooled
>> or pooled with a different size than the freeing code supplied,
>> preventing mysterious coredumps.
>>
>>
>>> The new code doesn't enforce
>>> this limit anymore: pools can handle the case of nonpool-ed blobs
>>> being pool-freed, and xfree is called for all other cases anyways.
>>
>> The proposed code does not check that the right pool is used anymore.
>> This is a negative side-effect of your changes. Is it worth the
>> benefits? Depends on what the alternatives are.
>
> Pools should be able to handle this case in a robust manner; in fact
> this is a code-path which can be hit if a MemBlob allocates a chunk of
> memory which has the same size of a pool before pools are initialized.

Current code does not support early allocations (pre-confgiguration
pooling). It is a good idea to improve that, of course, but I do not
think that requires removing the existing checks. If you propose to
remove them, let's discuss whether that is the best way forward.

> The case at hand is supporting globals and mempools, and selecting the
> right destructor if mempools are not available,

Sure.

> without using flags in MemBlob.

That part is not a use case. That is one possible solution.

> The original plan was to act exactly as you suggest later on in the
> mail (I'll call it "size-flagging and quoting") for brevity. The
> assertion needs to go anyways, or it'll be hit for objects which are
> small enough to fit in a pool, but not pooled.

AFAICT, if either of my suggestions is implemented, the assertion can
stay: All early allocations will be too big to trigger it and the
flagged MemBlob would not call this code at all.

> During discussions on IRC it was suggested that it may be not needed,
> as pools should be able to handle such corner (we may call it "error")
> cases.

The assertion was there to check for coding bugs. Removing it without
replacement will expose us to those bugs. Removing it may also help
cover more use cases, but that does not necessarily mean that the
assertion "is not needed".

>> Please consider leaving the current safety checks in place and just
>> allocating 2*M or M+1 size strings when pools are not initialized, where
>> M is the maximum supported string pool size. This will pass all
>> memFreeString checks and automatically use xfree for those "early
>> allocations", right?
>
> Yes. That was the original plan. I will implement it. Just making sure
> that the size of the allocated blob is an odd number will do.

Using odd numbers will make the current check less effective -- it will
have to be bypassed whenever the submitted size is odd (which could be
due to the "needed" size given to memFreeString instead of "allocated").
Given that StringNG is about to change a lot of memory-related code, I
think it would be better to leave the check as is and sacrifice a few
KBs of RAM instead.

>> In the unlikely event of the above approach creating too much RAM waste
>> due to huge volume of pre-pooled allocations, we can instead add a flag
>> to MemBlob so that it knows to use xfree instead of memFreeString for
>> "early allocations".
>
> This would be worse. Size-implied flagging wastes at most one word per
> global object. A flag to MemBlob means most likely one extra word per
> MemBlob (global or not), which is much more.

I am not sure one word per MemBlob is more waste than a few KBs per
early allocation. It depends on how much we end up allocating early.
However, avoiding flags may be better because it allows non-MemBlob
users to allocate early.

> Next steps:
> - implement size-quoting in memAllocString
> - implement a couple more string sizes ("big" 4k and "huge" 16k?)
> - test and resubmit

Sounds good to me, if size-quoting means making early allocations bigger
than the maximum supported string pool.

Thank you,

Alex.
Received on Wed Mar 30 2011 - 18:48:18 MDT