Re: StringNg review: MemBlob

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:

MemBlob and SBuf are meant to be used together. They split the burden
of managing memory correcly.

> Comments for src/MemBlob.h r9472:
>
>> #include "MemPool.h"

Needed for MEMPROXY_CLASS etc

> Remove if not needed.
>
>> #include "RefCount.h"
>>
>> /**
>>  * A container for various MemBlob class-wide statistics
>>  */
>> class MemBlobStats {
>>     public:
>>     u_int64_t alloc; ///number of allocation operations
>
> Class allocations or buffer allocations?

Clarified. MemBlob allocations

>>     u_int64_t realloc; ///number of re-allocation operations
>
> .
>
>>     u_int64_t live; ///number of live references to bufs
>
> How can you know the number of references? Do you mean the number of MemBob
> instances in existence?

Number of MemBlob instances. Clarified.

>
>>     u_int64_t append; ///number of append operations (whtout grow)
>
> spelling
> Besides, MemBob does not grow!

Fixed

>>     u_int64_t allocatedBytes; ///the total size of the allocated storage
>
> Conflicts with the previous use of "allocation". If that is not changed,
> use liveBytes here.

Done.

>>     /**
>>      * Dumps statistics to an ostream. Use an ostringstream to
>>      * capture them to a string
>>      */
>>     std::ostream& dump(std::ostream& os) const;
>
> Consider removing reference to ostringstream or moving it to some .dox file.
> It does not belong to interface documentation.

Fixed.

>>     MemBlobStats();
>> };
>
> .
>
>> /**
>>  * Low-level buffer. It's responsible for allocating and managing
>>  * the blocks of memory. It's refcounted, but it doesn't know
>>  * which SBufs hold references to it.
>>  */
>
> MemBlob does not manage blocks, it manages one block. It does not know many
> things (do not document what is not there).

Fixed

>
> Consider:
> /// A refcounted fixed-size buffer. Tracks the portion used by opaque
> content.

How about:
"Refcounted memory buffer, content-agnostic. Meant to only be used by SBuf." ?

>> class MemBlob: public RefCountable {
>>     public:
>>     typedef RefCount<MemBlob> Pointer;
>>     typedef unsigned int size_type;
>
> Would be nice to guarantee support for 5GB and larger blobs.

Do we need them now? It'll be trivial to extend when needed.

>>     char *mem;         /// The block of memory
>
> Missing "<" in Doxygen comments?

Yes. I wasn't aware of the construct.

>
> Consider: ///< raw allocated memory
>
>>     size_type bufSize; /// Size of the allocated memory
>>     size_type bufUsed; /// How much of the memory is actually used
>
> Other Squid code is using capacity and size for this. Std::string also uses
> size() for user-facing buffer length. Please be consistent.

Changed, even though I find the name "size" confusing in this case.

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

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

>>     MemBlob(const size_type size);
>
> Missing "explicit". It is better to document whether it allocates anything.

Fixed.

>>     MemBlob(char * memBlock, const size_type memSize);
>
> Not sure why this is needed, but needs to be documented, especially with
> regard to memBlock ownership and destruction rules.

Documented.

> Please keep in mind that without a default constructor (and reserve()), it
> would be impossible to have containers of blobs.

Not needed. We want containers of SBufs, a MemBlob is more akin to a
"smart struct".

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

>>     _SQUID_INLINE_ size_type freeSpace() const;
>
> Missing documentation.

Renamed to getAvailableSpace. Documentation was in the .cci, moved it

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

> After reading MemBuf.cc: MemBlob cannot grow, apparently! Replace this with
> something like willFit(sizeToAppend) or canAppend(sizeToAppend).

willFit is a good name. Renaming.

> s/howmuch/byHowMuch/ or s/howmuch/toHowMuch/, depending on what this does.
>
>
>>     _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.

>>     _SQUID_INLINE_ void append(const char *S, const size_type Ssize);
>
> Missing documentation.

moved from .cci and (hopefully) clarified

> Comments for MemBlob.cci r9472:
>
> * Please move methods longer than one line to .cc.

De-inlined memAlloc. I'd live mem

> * Please move public method comments to .h per current documentation rules.
>
> * MemBlob::append() should check boundaries and throw.
>
>
>
>
> Comments for MemBlob.cc r9472:
>
>> #include "config.h"
>> #include "MemBlob.h"
>> #include "Debug.h"
>
> M and D are out of order per current #include ordering rules.

Fixed.

>> MemBlobStats MemBlob::stats;
>
> If some static code wants to allocate MemBlob, will its stats not get
> counted, depending on static initialization order?

Yes. I don't think that's much of an issue; stats are mostly meant to
help us diagnose the effectiveness of the solution, and in fact I
wouldn't be against #ifdef-ing them out in production code.

>>             "\nMemBlob average size: " <<
>> (static_cast<float>(allocatedBytes)/live) << std::endl;
>
> Handle the case of live == 0.

done

> Please do not use float. Use double.

done.

> s/average/mean/ to be more precise.

ok

> The text label is a little misleading because you compute the average based
> on currently alive blobs only.

changed

>> /**
>>  * 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?)

> Please use construction/destruction comments style used by AsyncJob and many
> other Squid classes. Debugging consistency makes leak tracking using
> scripts/find-alive.pl much easier:
>    "MemBlob constructed, this=" << this << " size=" << size
>    "MemBlob destructed, this=" << this << " size=" << size

Done.

>> /**
>>  * Importer from the unmanaged world. Takes as argument a char* and a
>> length,
>>  * and builds a managed MemBlob out of those.
>>  */
>
> Use exact "copy" instead of vague "import".

Done.

>> MemBlob::MemBlob(char * memBlock, const MemBlob::size_type memSize)
>> {
>
> Must use "const char *memBlock" to avoid ownership questions I asked above.

Done.

-- 
    /kinkie