Re: StringNg review: MemBlob

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:.

>
>>> _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".

>>> _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

or rather isAtEnd being a case of canAppend(0)

>> 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.

>>> _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.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2