Re: StringNg review: MemBlob

On Wed, Sep 22, 2010 at 5:31 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 22/09/10 21:30, Kinkie wrote:
>>
>> On Tue, Sep 21, 2010 at 4:18 AM, Alex Rousskov
>> <rousskov_at_measurement-factory.com>  wrote:
>>>
>>> On 09/03/2010 09:21 AM, Kinkie wrote:
>>>
>>>> You'll find the branch at lp:~kinkie/squid/stringng
>>>
>>> I decided to take a look at MemBlob because it is used by SBuf which was
>>> reviewed a few days ago. It would be nice to get a fixed SBuf committed,
>>> but
>>> that would require committing the classes it uses.
>>>
>>> Committing smaller self-contained parts may be an overall better strategy
>>> than trying to fix everything first and then do one huge commit.
>>>
>>> So here is a MemBlob review:
>>
> <snip>
>>>
>>> Remove the "buf" prefix until you have some other size and capacity
>>> members
>>> to distinction from. Besides, this is not a "buf". This is a "blob" :-)
>>
>> Yup. Relics... I've been working on this for more than two years now.
>>
>>> I would not leave these data members public, but it is your call.
>>
>> This class originally was private to SBuf. I've now clarified in the
>> class definition that it's not meant for general use; its only client
>> should be SBuf.
>
> If still relevant that would be reason for protected: and friend
> declarations in the class definition instead of public:.

I'm fine either ways, the only client I'm concerned with is SBuf.

>>>>     _SQUID_INLINE_ void deleteSelf();
>>>
>>> I hope this can be removed. If not, it must be documented. The
>>> corresponding
>>> comments in .cci do not make sense to me and do not match RefCount.h in
>>> trunk, as far as I can tell.
>>
>> http://old.squid-cache.org/Doc/Prog-Guide/prog-guide-24.html
>> http://markmail.org/message/kt7357pwttmzrey3
>>
>> Maybe the documentaiton is outdated?
>>
>
> Seems to be. The RefCountable classes no longer have "deleteSelf".

removed.

>>>>     _SQUID_INLINE_ bool isAtEnd(const char *ptr) const;
>>>
>>> Missing documentation.
>>> Why is this needed? Some transitional call? If yes, document as such.
>>
>> It is needed to know whether a the tail of a SBuf is at the tail of a
>> shared MemBlob. Such a SBuf can append without needing a cow, if
>> there's enough headroom in the MemBlob.
>
> So, a char* way of saying:
>
>  if (SBuf::off_+Sbuf::len_ == MemBlob::size &&
>     SBuf::off_+Sbuf::len_ < MemBlob::capacity) {
>  ...
>
> with canAppend being a duplicate as above but <= MemBlob::capacity+N

canAppend is exactly "isAtEnd && willFit" (isAtEnd is always true if
RefCount==1)

> or rather isAtEnd being a case of canAppend(0)

Well, yes: willFit(0) is always true by definition (you can always
append nothing).

>>> This reads like a command to free some space.
>>> Consider: spaceSize()
>>> spaceSize() would be more consistent with other Squid code as well.
>>>
>>>
>>>>     _SQUID_INLINE_ bool canGrow(const size_type howmuch) const;
>>>
>>> Missing documentation.
>>> Usually unsafe and wastefull. See similar comments for SBuf.
>>
>> It'd be unsafe in a concurrent (multithreaded) environment; how would
>> it be unsafe in a single-threaded env?
>>
>
> Squid won't be single-threaded for much longer. At least on the timescale
> for string-level changes.

Most String/Buf implementations avoid locking, there's too much
overhead to be spent for such a low-level function, it's usually
better dealt with on an higher level.

>>>>     _SQUID_INLINE_ bool canAppend(char const * bufEnd, const size_type
>>>> howmuch) const;
>>>
>>> Missing documentation.
>>> Especially since it is not clear why canAppend() needs a pointer.
>>> Looking at the .cci implementation, I question why would the caller
>>> keep the bufEnd pointer if MemBlob manages it already? I do not think
>>> this
>>> call or duplicated "bufEnd" pointer is needed.
>>
>> if many SBufs reference the same MemBlob, an append operation to one
>> of them does not need a cow if the appended-to SBuf is at the tail of
>> the MemBlob and there is enough free space at the end of the MemBlob
>> to hold the data being appended.
>> This canAppend call checks exactly that. the SBuf knows its own tail,
>> while the MemBlob knows the tail of the used portion of itself.
>>
>
> see isAtEnd() comment above.
>
>>>> /**
>>>>  * constructor, creates an empty MemBlob of size>= requested
>>>>  * \param size the minimum size to be guarranteed from the MemBlob
>>>>  */
>>>> MemBlob::MemBlob(const MemBlob::size_type size)
>>>> {
>>>>     debugs(MEMBLOB_DEBUGSECTION,9,HERE<<  "size="<<  size);
>>>>     memAlloc(size);
>>>> }
>>>
>>> I would recommend initializing mem and sizes to NULL/0s explicitly in all
>>> the constructors.  It does not cost much and may save a few hours of
>>> debugging if things go wrong.
>>
>> Isn't setting them in memAlloc enough? Besides the actual size may be
>> different (or would you mean to set them twice?)
>
> In the constructor init list.
>
> You can then enforce the MUST and ONLY requirements with assert(mem==NULL)
> etc at the start of memAlloc.

Done.
Thanks.

-- 
    /kinkie