Re: [PATCH] Restore mempools functionality for MemBlob

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.

> +// TODO: have MemPools do system page size magic. They don't really belong here

but the corresponding patched code does not refer to the "system page
size" anymore. Delete this TODO? More on this code below.

> MemBlob::calcAllocSize(const size_type sz) const
> {
> if (sz <= 36) return 36;
> if (sz <= 128) return 128;
> if (sz <= 512) return 512;
> if (sz <= 4096) return RoundTo(sz, 512);
> - // XXX: recover squidSystemPageSize functionality. It's easy for
> - // the main squid, harder for tests
> -#if 0
> - return RoundTo(sz, squidSystemPageSize);
> -#else
> return RoundTo(sz, 4096);
> -#endif
> }

Why do we need the above if memAllocString() already has similar logic
and can overwrite our decision anyway?

> - assert(size && buf);
> + assert(buf);
>
> for (i = 0; i < mem_str_pool_count; i++) {
> - if (size <= StrPoolsAttrs[i].obj_size) {
> - assert(size == StrPoolsAttrs[i].obj_size);
> - pool = StrPools[i].pool;
> + if (size > StrPoolsAttrs[i].obj_size)
> + break;
> + if (size == StrPoolsAttrs[i].obj_size) {
> + pool = StrPools[i].pool;
> break;
> }

The above change does not make sense to me. My understanding is that the
undocumented memFreeString() function expects the actual allocated size
as the parameter. It relies on that expectation to find the right pool.
Thus, the removed code was correct. Moreover, we do supply the right
size value in the destructor.

I may be missing something, but it feels like we just need to take care
of the case where memory allocation failed (due to an exception or
whatnot), leaving capacity and buf zero in the destructor:

MemBlob::~MemBlob()
{
    if (capacity || mem) // memFreeString detects any inconsistencies
        memFreeString(capacity,mem);
    ...
}

> -#if HAVE_OSTREAM
> -#include <ostream>
> +#if HAVE_IOSFWD
> +#include <iosfwd>
> #endif

Is this change related to the "increased flexibility"? If not, please
remove. If the change is needed for other reasons, it is OK to commit it
separately, of course.

> @@ -70,7 +70,6 @@
> }
>
> StrPoolsAttrs[mem_str_pool_count] = {
> -
> {
> "Short Strings", MemAllocator::RoundedSize(36),
> }, /* to fit rfc1123 and similar */
...
> - int i;
> + int i;

Please avoid whitespace changes.

Please include proposed commit message as the patch preamble (or in the
email text).

Please use 20-line diffs (-U 20) for patches if possible. Seeing more
context often makes it easier to review the changes.

Thank you,

Alex.
Received on Wed Mar 30 2011 - 15:46:46 MDT