Re: StringNg review: MemBlob

Hi Kinkie,

     Here is another review you have requested. This review is based on
lp:~kinkie/squid/stringng revision 9504.

> #include "base/TextException.h"

Remove if not needed in MemBlob.h

> class MemBlobStats {
...
> };

Not reviewed per Kinkie's request.

> /**
> * Refcounted memory buffer, content-agnostic. Meant to only be used
> * by SBuf.
> */

Remove "Meant to only be used by SBuf." This class should be generally
usable.

> class MemBlob: public RefCountable {
> public:

Still missing source formatting.

> char *mem; ///< Raw memory block
> size_type capacity;///< Size of the memory block
> size_type size; ///< How much of the memory is actually used

Start comment with a small letter or end with a period. I recommend the
former for short comments that are usually not full sentences.

> //do not meddle with. Use getStats to obtain a copy for dumping
> static MemBlobStats stats;
> static const MemBlobStats& getStats();

Both missing documentation. What not to do instructions are not enough.

I do not recall whether we have discussed this already, but if you can
make the stats member private, please do.

getStats() name should start with a capital letter since it is static.

> /**
> * Create a new MemBlob, containing a copy of the
> * contents of the buffer of size memSize
> * pointed to by memBlock. It is up to the caller to
> * manage the memBlock
> */
> MemBlob(const char * memBlock, const size_type memSize);

Remove "It is up to ..." because const pointer already implies that.

> /** check whether the supplied pointer is the first unused byte in the MemBlob
> */
> _SQUID_INLINE_ bool isAtEnd(const char *ptr) const;

nitpick: the "supplied pointer" is not a "byte".

Please document whether this returns true when there are no unused
MemBblob bytes. I still do not like this interface itself though. There
must be a cleaner way to do what you want. How about this:

     /// whether writing to the [start, start + n) area is currently OK
     bool writable(const char *start, size_type n) const;

This would both solve the problem with the lack of unused bytes and
check for exclusive control of the area. Would that work for the current
callers?

Moreover, do we need this method, willFit(), and canAppend()? They all
do pretty much the same kind of checks, from different angles, with
different bugs, under very different names. This needs to be polished,
but I do not know which end to start from. Can you reduce the problem space?

> /** check whether we can append data at the tail of the MemBlob
> *
> * \param toAppend number of bytes to be appended
> * \return true there is at least toAppend free space available
in the MemBlob
> */
> _SQUID_INLINE_ bool willFit(const size_type toAppend) const;

.

> /** return the number of bytes available at the tail of the MemBlob
> *
> */

/// the number of allocated but unused bytes at the end of the blob

> _SQUID_INLINE_ size_type getAvailableSpace() const;

Wrong name. The method does not give the caller the available space,
only its size. Here are some combinations that work better:

    size(), spaceSize(), capacity()
    contentSize(), spaceSize(), capacity()
    usedSize() and availableSize(), capacity()

> /** check whether we can append data at the tail of the MemBlob
> *
> * \param toAppend number of bytes to be appended
> * \return true there is at least toAppend free space available in the MemBlob
> */
> _SQUID_INLINE_ bool willFit(const size_type toAppend) const;

.

> /** check if there is enough unused space at the tail of the MemBlob
> *
> * \return true the append would fit
> * \param bufEnd first unused byte after the end of the SBuf being appended to
> * \param toAppend how many bytes would be nice to append
> */
> _SQUID_INLINE_ bool canAppend(char const * bufEnd, const size_type toAppend) const;

MemBlob should not know about SBuf; please remove references to SBuf.

If we are really _appending_ to MemBlob, we should not have to specify
the end of the used buffer area because MemBlob knows where it ends. See
above for general comments and suggestions to fix the will/can/is* mess:
We need fewer append-related members with a consistent naming scheme.

> /** append a c-string by copy and explicit size

Consider:
* Appends exactly n bytes starting with S. Does not check for
* null-termination of S. Throws if there is not enough space.

> * It is responsibility of the caller to make sure that there is
> * enough space

What happens if there is not? I think we should throw. This needs to be
documented. See above for a suggestion.

> * \param S the char* to be appended at the end of MemBlob

"at the end of MemBlob" can be interpreted as the end of allocated area.
Use "after the used area" or similar.

> * \param Ssize the length of the array to be appended
> */
> _SQUID_INLINE_ void append(const char *S, const size_type Ssize);

Parameter names should start with a small letter, IIRC. Consider "buf"
and "n".

> _SQUID_INLINE_ size_type memAllocationPolicy(const size_type size) const;

Why is the allocation size called "policy"? The policy could be the
algorithm that computes the size, but not the size itself. Use something
like calcAllocSize().

> /**
> * Given a requested size, return a rounded allocation size to be used
> * for the backing store.
> * This is a stopgap call, this job is eventually expected to be handled
> * by MemPools via memAllocString.
> */
> MemBlob::size_type
> MemBlob::memAllocationPolicy(const MemBlob::size_type size) const

Do we need MemBlob:: inside parens? Please remove if we do not. Here and
in other methods.

This method should not be inlined. The following malloc is expensive
anyway and we do not want to drag memory allocation dependencies to users.

> {
> if (size < 128) return 128;

Should not this be <= 128?

> if (size < 512) return 512;
> if (size < 1024)
> return RoundTo(size,128);
> if (size < 4096)
> return RoundTo(size,512);

Same here.

> bool
> MemBlob::willFit(const MemBlob::size_type toAppend) const
> {
> return toAppend < getAvailableSpace();
> }

Should not this be <=?

Can you just place that in the .h file, inlining during the declaration?

> bool
> MemBlob::canAppend(char const *bufEnd, const MemBlob::size_type toAppend) const
> {
> return isAtEnd(bufEnd) && willFit(toAppend);
> }

Why cannot I append if bufEnd is not at the end of the used area?

Can you just place this and similar basic one-line methods in the .h
file, inlining them during the declaration? If you do that and move
expensive methods to .cc, then we will not have to deal with MemBlob.cci
at all!

> void
> MemBlob::append(const char *S, const MemBlob::size_type Ssize)
> { /** \note memcpy() is safe because we copy to an unused area
> */

Use /// comments for one-line comments or at least do not make them
two-line.

> memcpy(mem+size,S,Ssize);
> size+=Ssize;
> ++stats.append;
> }

Please add a Must(!overflow) check.
Please add a NULL S check.

This method should not be inlined.

> /**
> * constructor, creates an empty MemBlob of size >= requested
> * \param size the minimum size to be guarranteed from the MemBlob
> */

Duplicate documentation? This is a public method, right?

> MemBlob::MemBlob(const MemBlob::size_type reserveSize) :
> mem(NULL), capacity(0), size(0) // will be set by memAlloc
> {
> memAlloc(reserveSize);
> debugs(MEMBLOB_DEBUGSECTION,9, "MemBlob constructed, this="
> << static_cast<void*>(this)
> << " reserveSize=" << reserveSize
> << " capacity=" << capacity);
> }
>
>
> /**
> * Copy a memory block from the unmanaged world.
> * Takes as argument a char* and a length, and builds a managed
> * MemBlob out of those. It is up to the caller to free the passed
> * block.
> */

Duplicate documentation? This is a public method, right?

> MemBlob::MemBlob(const char * memBlock, const MemBlob::size_type memSize) :
> mem(NULL), capacity(0), size(0) // will be set by memAlloc
> {
> memAlloc(memSize);
> append(memBlock,memSize);
> debugs(MEMBLOB_DEBUGSECTION,9, "MemBlob constructed, this="
> << static_cast<void*>(this)
> << " memBlock=" << static_cast<const void*>(memBlock)
> << " memSize=" << memSize << " capacity=" << capacity);

Move debugs() higher, at least before the append() which may throw.

>
> }
>
> MemBlob::~MemBlob()
> {
> debugs(MEMBLOB_DEBUGSECTION,9, "MemBlob destructed, this="
> << static_cast<void*>(this)
> << " capacity=" << capacity
> << " size=" << size);
> --stats.live;
> stats.liveBytes-=capacity;
> #if MEMBLOB_USES_MEM_POOLS
> //no mempools for now
> // \todo reinstate mempools use
> memFreeString(capacity,mem);
> #else
> xfree(mem);
> #endif
> }

Move debugs() as low as you can.

> /**
> * create a new empty MemBlob the requested size
> * To be called only by constructors
> *
> * The block is NOT initialized.
> */
> void
> MemBlob::memAlloc(const MemBlob::size_type memSize)
> {
> size_t actualAlloc;

Please initialize (to zero).

Add Must(!mem).

> #if MEMBLOB_USES_MEM_POOLS
> //for now, do without mempools
> mem=(char *)memAllocString(memSize,&actualAlloc);
> #else
> //\todo reinstate mempools use
> actualAlloc=memAllocationPolicy(memSize);
> mem=static_cast<char*>(xmalloc(actualAlloc));
> #endif
> Must(mem);
> capacity=actualAlloc;
> size=0;
> debugs(MEMBLOB_DEBUGSECTION,8,
> HERE << "this=" <<(void *)this <<", requested=" << memSize <<
> ", received=" << capacity);
> ++stats.live;
> ++stats.alloc;
> stats.liveBytes+=capacity;
> }

Thank you,

Alex.
Received on Wed Oct 06 2010 - 22:49:54 MDT