Re: StringNg review: MemBlob

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:

Comments for src/MemBlob.h r9472:

> #include "MemPool.h"

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?

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

> u_int64_t append; ///number of append operations (whtout grow)

spelling
Besides, MemBob does not grow!

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

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

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

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

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

> char *mem; /// The block of memory

Missing "<" in Doxygen comments?

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.

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" :-)

I would not leave these data members public, but it is your call.

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

> MemBlob(const size_type size);

Missing "explicit". It is better to document whether it allocates anything.

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

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

> _SQUID_INLINE_ bool isAtEnd(const char *ptr) const;

Missing documentation.
Why is this needed? Some transitional call? If yes, document as such.

> _SQUID_INLINE_ size_type freeSpace() const;

Missing documentation.

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.

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

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.

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

Missing documentation.

Comments for MemBlob.cci r9472:

* Please move methods longer than one line to .cc.

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

> MemBlobStats MemBlob::stats;

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

> "\nMemBlob average size: " <<
(static_cast<float>(allocatedBytes)/live) << std::endl;

Handle the case of live == 0.

Please do not use float. Use double.

s/average/mean/ to be more precise.

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

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

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

> /**
> * 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".

> MemBlob::MemBlob(char * memBlock, const MemBlob::size_type memSize)
> {

Must use "const char *memBlock" to avoid ownership questions I asked above.

HTH,

Alex.
Received on Tue Sep 21 2010 - 02:19:20 MDT