Re: [PATCH] Restore mempools functionality for MemBlob

On 03/30/2011 01:23 PM, Kinkie wrote:

>>> 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.
>
> Well, that's a bit much to ask for.. it would mean a 16kb+ allocation
> for a 1byte global.

If we only allocate "a few" globals, the overhead would be negligible.
And if we allocate "a lot", perhaps the proper fix would be to actually
pool them properly.

> Here is an excerpt of the current code for your evaluation.
> -------------------------
> void *
> memAllocString(size_t net_size, size_t * gross_size)
> {
> MemAllocator *pool = NULL;
> assert(gross_size);
>
> unsigned int i;
> for (i = 0; i < mem_str_pool_count; ++i) {
> if (net_size <= StrPoolsAttrs[i].obj_size) {
> pool = StrPools[i].pool;
> break;
> }
> }
>
> // we haven't found a pool even though we should have, this
> // means that the pools haven't been initialized yet. Increase size
> // so that we don't accidentally hit a pool size in memFreeString
> if (!pool && StrPoolsAttrs[i].obj_size == net_size )
> ++net_size;

If you goal is to support early allocations, then the above does not
make sense to me on many levels:

* If pools are indeed not initialized (and they would not be during some
static allocations), we should not access StrPoolsAttrs at all! There
are different initialization stages here, and you have to handle the all
of them for the code to work reliably.

* If we did not find the pool, "i" may point beyond the StrPoolsAttrs array.

* We could find no pool because there is no pool large enough for our
net_size and not because pools have not been initialized yet. This can
be fixed by starting the comment with "If " and rewarding the rest of
the comment, I guess.

Etc.

> void
> memFreeString(size_t size, void *buf)
> {
> MemAllocator *pool = NULL;
> assert(buf);
>
> for (unsigned int i = 0; i < mem_str_pool_count; ++i) {
> if (size == StrPoolsAttrs[i].obj_size) {
> pool = StrPools[i].pool;
> break;
> }
> }
>
> memMeterDec(StrCountMeter);
> memMeterDel(StrVolumeMeter, size);
> pool ? pool->freeOne(buf) : xfree(buf);
> }

And to add insult to injury, we now completely lost the check that
pooled strings are not xfreed due to wrong size being given to
memFreeString().

> This code is build-tested and passes the test-suite.

Which says a lot about the quality of that test-suite...

Alex.
Received on Wed Mar 30 2011 - 19:59:38 MDT