Re: StringNg review: MemBlob

On Mon, Oct 18, 2010 at 7:56 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> Hi Kinkie,
>
>    Attached is am untested patch with some final polishing changes for
> MemBlob, based on lp:~kinkie/squid/stringng revision 9517. I think the
> MemBlob class should be committed to trunk after these changes are tested
> and polished (I may have missed a few minor problems when moving or renaming
> stuff). The rest of the branch code would need to be synced as well.

Polished and build-tested.
Unless someone objects, I'm going to merge MemBlob to trunk this evening (GMT).
It lacks unit-tests (SBuf's unit tests provide them indirectly).

>
> The patch preamble documents the major changes.
>
> While working on this, I realized that the "MemBlob::size" member is only an
> approximation, and that MemBlob class is probably not usable for efficient
> message body pipelining (BodyPipe and friends). We will probably change
> MemBlob API later to accommodate users other than strings, but that can
> wait. I adjusted the append call to remove the use of pointers which may
> make future changes less disruptive.

Fixed SBuf accordingly.

> Once MemBlob is committed, please consider re-integrating it with memory
> pools. This is the biggest remaining XXX, IMO.

I've checked and confirmed why MemPools integration was disabled: it's
due to an inherent asymmetry in memAllocString/memFreeString: if
mempools are not initialized, memAllocString will happily allocate the
string using xcalloc. When the time comes to deallocate such strings,
it will fail to find a pool to dealloc from and fail an assertion.

The only way I can think of to cleanly resolve this is quite
disruptive, as it means making MemPools a proper singleton obect to
make sure they are initialized before being used: it's a complex
project on its own which I'd list as a dependency before integrating
MemPools back in.

IMVHO body pipelining should go through SBuf, not MemBlob, but that's
for body pipelining to work out.

-- 
    /kinkie