Hi Kinkie,
Attached is am untested patch with some final polishing changes for
MemBlob, based on lp:~kinkie/squid/stringng revision 9517. I think the
MemBlob class should be committed to trunk after these changes are
tested and polished (I may have missed a few minor problems when moving
or renaming stuff). The rest of the branch code would need to be synced
as well.
The patch preamble documents the major changes.
While working on this, I realized that the "MemBlob::size" member is
only an approximation, and that MemBlob class is probably not usable for
efficient message body pipelining (BodyPipe and friends). We will
probably change MemBlob API later to accommodate users other than
strings, but that can wait. I adjusted the append call to remove the use
of pointers which may make future changes less disruptive.
Once MemBlob is committed, please consider re-integrating it with memory
pools. This is the biggest remaining XXX, IMO.
Thank you,
Alex.
Got rid of MemBlob.cci because one member should not have been inlined (it is
complex code, with complex linking dependencies, that is used as a part of an
expensive call) and the other members are one-liners that are better
inlined directly (less time spent finding and understanding the code).
Replaced raw pointers with offsets in canAppend(). Current MemBlob users
already use offsets to compute those pointers anyway. And future MemBlobs may
support sliding use window via ever-incrementing offsets, which will make raw
pointer use even less appropriate. This change effectively removes the
implicit "am I using the blob I think I am using?" check. I think it is OK to
remove that because if some user is so seriously confused, we should not try
to mask the problem at the canAppend() level.
Added clear() method so that users can reset the blob without modifying size
member directly. Users still need access to reference count to know when
clearing is safe, unfortunately. We may add something like clearIfLonely()
later.
Documented the two blob areas and explained how they change. Documented lack
of sharing protections beyond blob refcounting. Documented that "size" member
is only approximate because it is not decreased when users leave.
Polished creation/destruction messages to match find-alive patterns.
Polished method and method argument names.
Polished comments, made comments more consistent, and applied a few
Source Formatting rules.
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2010-10-15 10:22:29 +0000
+++ src/Makefile.am 2010-10-18 17:05:43 +0000
@@ -543,8 +543,7 @@
String.cci \
SquidString.h \
SquidTime.h \
- SBuf.cci \
- MemBlob.cci
+ SBuf.cci
BUILT_SOURCES = \
cf_gen_defines.cci \
=== modified file 'src/MemBlob.cc'
--- src/MemBlob.cc 2010-10-14 22:31:46 +0000
+++ src/MemBlob.cc 2010-10-18 17:17:18 +0000
@@ -37,68 +37,55 @@
#include <iostream>
#endif
-MemBlobStats MemBlob::stats;
-InstanceIdDefinitions(MemBlob, "MemBlob");
-
-MemBlobStats::MemBlobStats() : alloc(0), live(0), append(0)
+#define MEMBLOB_USES_MEM_POOLS 0
+
+MemBlobStats MemBlob::Stats;
+InstanceIdDefinitions(MemBlob, "blob");
+
+
+/* MemBlobStats */
+
+MemBlobStats::MemBlobStats(): alloc(0), live(0), append(0)
{}
-const MemBlobStats&
-MemBlob::GetStats()
-{
- return stats;
-}
-
std::ostream&
-MemBlobStats::dump(std::ostream& os) const
+MemBlobStats::dump(std::ostream &os) const
{
os <<
- "MemBlob allocations: " << alloc <<
- "\nMemBlob live references: " << live <<
- "\nMemBlob append operations: " << append <<
- "\nMemBlob total allocated volume: " << liveBytes <<
- "\nlive MemBlobs mean size: " <<
+ "MemBlob created: " << alloc <<
+ "\nMemBlob alive: " << live <<
+ "\nMemBlob append calls: " << append <<
+ "\nMemBlob currently allocated size: " << liveBytes <<
+ "\nlive MemBlob mean current allocation size: " <<
(static_cast<double>(liveBytes)/(live?live:1)) << std::endl;
return os;
}
-std::ostream&
-MemBlob::dump (std::ostream &os) const
-{
- os << "id @" << (void *)this
- << "mem:" << (void*) mem
- << ",size:" << capacity
- << ",used:" << size
- << ",refs:" << RefCountCount() << "; ";
- return os;
-}
+/* MemBlob */
MemBlob::MemBlob(const MemBlob::size_type reserveSize) :
mem(NULL), capacity(0), size(0) // will be set by memAlloc
{
- debugs(MEMBLOB_DEBUGSECTION,9, id << "constructed, @"
- << static_cast<void*>(this)
- << ", reserveSize=" << reserveSize);
+ debugs(MEMBLOB_DEBUGSECTION,9, HERE << "constructed, this="
+ << static_cast<void*>(this) << " id=" << id <<
+ << " reserveSize=" << reserveSize);
memAlloc(reserveSize);
}
-MemBlob::MemBlob(const char * memBlock, const MemBlob::size_type memSize) :
+MemBlob::MemBlob(const char *buffer, const MemBlob::size_type bufSize) :
mem(NULL), capacity(0), size(0) // will be set by memAlloc
{
- debugs(MEMBLOB_DEBUGSECTION,9, id << "constructed, @"
- << static_cast<void*>(this)
- << ", memBlock=" << static_cast<const void*>(memBlock)
- << ", memSize=" << memSize);
- memAlloc(memSize);
- append(memBlock,memSize);
-
+ debugs(MEMBLOB_DEBUGSECTION,9, HERE << "constructed, this="
+ << static_cast<void*>(this) << " id=" << id <<
+ << " buffer=" << static_cast<const void*>(buffer)
+ << " bufSize=" << bufSize);
+ memAlloc(bufSize);
+ append(buffer, bufSize);
}
MemBlob::~MemBlob()
{
- --stats.live;
- stats.liveBytes-=capacity;
#if MEMBLOB_USES_MEM_POOLS
//no mempools for now
// \todo reinstate mempools use
@@ -106,52 +93,92 @@
#else
xfree(mem);
#endif
- debugs(MEMBLOB_DEBUGSECTION,9, id <<" destructed"
- << ", capacity=" << capacity
- << ", size=" << size);
-}
-
-/** create a new empty MemBlob the requested size
- * Called by constructors.
- *
- * The block is NOT initialized.
+ stats.liveBytes -= capacity;
+ --stats.live;
+
+ debugs(MEMBLOB_DEBUGSECTION,9, HERE << "destructed, this="
+ << static_cast<void*>(this) << " id=" << id <<
+ << " capacity=" << capacity
+ << " size=" << size);
+}
+
+/**
+ * Given the requested minimum size, return a rounded allocation size
+ * 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::calcAllocSize(const size_type size) const
+{
+ if (size <= 128) return 128;
+ if (size <= 512) return 512;
+ if (size <= 1024) return RoundTo(size, 128);
+ if (size <= 4096) return RoundTo(size, 512);
+ // XXX: recover squidSystemPageSize functionality. It's easy for
+ // the main squid, harder for tests
+#if 0
+ return RoundTo(size, squidSystemPageSize);
+#else
+ return RoundTo(size, 4096);
+#endif
+}
+
+/** Allocate an available space area of at least minSize bytes in size.
+ * Must be called by constructors and only by constructors.
*/
void
-MemBlob::memAlloc(const MemBlob::size_type memSize)
+MemBlob::memAlloc(const size_type minSize)
{
- size_t actualAlloc;
- actualAlloc=calcAllocSize(memSize);
+ size_t actualAlloc = calcAllocSize(minSize);
- Must(mem==NULL);
+ Must(!mem);
#if MEMBLOB_USES_MEM_POOLS
- //for now, do without mempools
- mem=(char *)memAllocString(memSize,&actualAlloc);
+ // XXX: for now, do without mempools
+ mem = static_cast<char*>(memAllocString(minSize, &actualAlloc));
#else
- //\todo reinstate mempools use
- mem=static_cast<char*>(xmalloc(actualAlloc));
+ // \todo reinstate mempools use
+ mem = static_cast<char*>(xmalloc(actualAlloc));
#endif
Must(mem);
- capacity=actualAlloc;
- size=0;
- debugs(MEMBLOB_DEBUGSECTION,8,
- id << " memAlloc: requested=" << memSize <<
+
+ capacity = actualAlloc;
+ size = 0;
+ debugs(MEMBLOB_DEBUGSECTION, 8,
+ id << " memAlloc: requested=" << minSize <<
", received=" << capacity);
++stats.live;
++stats.alloc;
- stats.liveBytes+=capacity;
+ stats.liveBytes += capacity;
}
void
-MemBlob::append(const char *S, const MemBlob::size_type n)
-{ ///\note memcpy() is safe because we copy to an unused area
- Must(n <= capacity-size);
- Must(S != NULL);
- memcpy(mem+size,S,n);
- size+=n;
+MemBlob::append(const char *source, const size_type n)
+{
+ if (n > 0) { // appending zero bytes is allowed but only affects the stats
+ Must(willFit(n));
+ Must(source);
+ /// \note memcpy() is safe because we copy to an unused area
+ memcpy(mem + size, source, n);
+ size += n;
+ }
++stats.append;
}
-#if !_USE_INLINE_
-#include "MemBlob.cci"
-#endif
-
+
+const MemBlobStats&
+MemBlob::GetStats()
+{
+ return Stats;
+}
+
+std::ostream&
+MemBlob::dump(std::ostream &os) const
+{
+ os << "id @" << (void *)this
+ << "mem:" << static_cast<void*>(mem)
+ << ",capacity:" << capacity
+ << ",size:" << size
+ << ",refs:" << RefCountCount() << "; ";
+ return os;
+}
=== removed file 'src/MemBlob.cci'
--- src/MemBlob.cci 2010-10-08 13:44:26 +0000
+++ src/MemBlob.cci 1970-01-01 00:00:00 +0000
@@ -1,91 +0,0 @@
-/*
- * MemBlob.cci (C) 2009 Francesco Chemolli <kin...@squid-cache.org>
- *
- * SQUID Web Proxy Cache http://www.squid-cache.org/
- * ----------------------------------------------------------
- *
- * Squid is the result of efforts by numerous individuals from
- * the Internet community; see the CONTRIBUTORS file for full
- * details. Many organizations have provided support for Squid's
- * development; see the SPONSORS file for full details. Squid is
- * Copyrighted (C) 2001 by the Regents of the University of
- * California; see the COPYRIGHT file for full details. Squid
- * incorporates software developed and/or copyrighted by other
- * sources; see the CREDITS file for full details.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA.
- */
-
-#include "config.h"
-//#include "Mem.h" //for squidSystemPageSize
-#include "Debug.h"
-
-/**
- * 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::calcAllocSize(const size_type size) const
-{
- if (size <= 128) return 128;
- if (size <= 512) return 512;
- if (size <= 1024)
- return RoundTo(size,128);
- if (size <= 4096)
- return RoundTo(size,512);
- //todo: recover squidSystemPageSize functionality. It's easy for
- // the main squid, harder for tests
-#if 0
- return RoundTo(size,squidSystemPageSize);
-#else
- return RoundTo(size,4096);
-#endif
-}
-
-/** check whether the supplied pointer points to the first
- * unused byte in the MemBlob. It does not take capacity into account,
- * so it may return true even if we have no available space.
- */
-bool
-MemBlob::isAtEnd(const char *ptr) const
-{
- return ptr==(mem+size);
-}
-
-/** 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
- */
-MemBlob::size_type
-MemBlob::availableSize() const
-{
- return capacity-size;
-}
-
-bool
-MemBlob::willFit(const size_type toAppend) const
-{
- return toAppend <= availableSize();
-}
-
-bool
-MemBlob::canAppend(char const *bufEnd, const size_type toAppend) const
-{
- return isAtEnd(bufEnd) && willFit(toAppend);
-}
-
=== modified file 'src/MemBlob.h'
--- src/MemBlob.h 2010-10-14 22:31:46 +0000
+++ src/MemBlob.h 2010-10-18 17:21:25 +0000
@@ -32,36 +32,36 @@
#define SQUID_MEMBLOB_H_
#define MEMBLOB_DEBUGSECTION 24
-#define MEMBLOB_USES_MEM_POOLS 0
-#include "config.h"
#include "base/InstanceId.h"
#include "MemPool.h"
#include "RefCount.h"
-/**
- * A container for various MemBlob class-wide statistics.
- *
- * The stats are not completely accurate; they're mostly meant to
- * undestand whether Squid is leaking resources.
- */
+/// Various MemBlob class-wide statistics.
class MemBlobStats
{
public:
- u_int64_t alloc; ///<number of MemBlob allocation operations
- u_int64_t live; ///<current number of MemBlob instances
- u_int64_t append; ///<number of append operations (wihtout realloc)
- u_int64_t liveBytes; ///<the total size of the allocated storage
+ MemBlobStats();
- /**
- * Dumps class-wide statistics
- */
+ /// dumps class-wide statistics
std::ostream& dump(std::ostream& os) const;
- MemBlobStats();
+
+public:
+ u_int64_t alloc; ///< number of MemBlob instances created so far
+ u_int64_t live; ///< number of MemBlob instances currently alive
+ u_int64_t append; ///< number of MemBlob::append() calls
+ u_int64_t liveBytes; ///< the total size of currently allocated storage
};
-/**
- * Refcounted, fixed-size, content-agnostic memory buffer.
+
+/** Refcountable, fixed-size, content-agnostic memory buffer.
+ *
+ * Allocated memory block is divided into two sequential areas:
+ * "used memory" and "available space". The used area can be filled during
+ * construction, grows via the append() call, and can be clear()ed.
+ *
+ * MemBlob users can cooperate to safely share the used area. However, MemBlob
+ * provides weak use accounting and no sharing protections besides refcounting.
*/
class MemBlob: public RefCountable
{
@@ -71,71 +71,71 @@
MEMPROXY_CLASS(MemBlob);
- char *mem; ///<raw memory block
- size_type capacity;///<size of the memory block
- size_type size; ///<how much of the memory is actually used
-
- ///Obtain a const view of class-wide statistics.
+ /// obtain a const view of class-wide statistics
static const MemBlobStats& GetStats();
- ///Create a new MemBlob, referencing a memory block of at least reserveSize
+ /// create a new MemBlob with at least reserveSize capacity
explicit MemBlob(const size_type reserveSize);
- /** Create a new MemBlob, containing a copy of the
- * contents of the buffer of size memSize
- * pointed to by memBlock.
- */
- MemBlob(const char * memBlock, const size_type memSize);
-
- ~MemBlob();
-
- ///return the number of bytes available at the tail of the MemBlob
- _SQUID_INLINE_ size_type availableSize() 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 buffer 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;
-
- /** append a c-string by copy and explicit size
- *
- * Appends exactly n bytes starting with S. Does not check for
- * null-termination of S.
- * \throw TextException if there is not enough space in the MemBlob
- * \param S c-buffer to be appended at the end of the used part of the MemBlob
- * \param n length of the c-buffer
- */
- void append(const char *S, const size_type n);
-
- /// Dump debugging information
+ /// create a MemBlob containing a copy of the buffer of a given size
+ MemBlob(const char *buffer, const size_type bufferSize);
+
+ virtual ~MemBlob();
+
+ /// the number unused bytes at the end of the allocated blob
+ size_type spaceSize() const { return capacity - size; }
+
+ /** check whether the caller can successfully append() n bytes
+ *
+ * \return true the caller may append() n bytes to this blob now
+ * \param off the end of the blob area currently used by the caller
+ * \param n the total number of bytes the caller wants to append
+ */
+ bool canAppend(const size_type off, const size_type n) const
+ {
+ // TODO: ignore offset (and adjust size) when the blob is not shared?
+ return isAppendOffset(off) && willFit(n);
+ }
+
+ /** copies exactly n bytes from the source to the available space area,
+ * enlarging the used area by n bytes
+ *
+ * \throw TextException if there is not enough space in the blob
+ * \param source raw buffer to be copied
+ * \param n the number of bytes to copy from the source buffer
+ */
+ void append(const char *source, const size_type n);
+
+ /// extends the available space to the entire allocated blob
+ void clear() { size = 0; }
+
+ /// dump debugging information
std::ostream & dump(std::ostream &os) const;
+public:
+ /* MemBlob users should avoid these and must treat them as read-only */
+ char *mem; ///< raw allocated memory block
+ size_type capacity; ///< size of the raw allocated memory block
+ size_type size; ///< maximum allocated memory in use by callers
+ const InstanceId<MemBlob> id; ///< blob identifier
private:
-
- static MemBlobStats stats;
+ static MemBlobStats Stats; ///< class-wide statistics
void memAlloc(const size_type memSize);
- _SQUID_INLINE_ size_type calcAllocSize(const size_type size) const;
-
- // do not implement those, allocation must be explicit
- MemBlob(const MemBlob & s);
- MemBlob& operator =(const MemBlob &s);
-
-
- _SQUID_INLINE_ bool isAtEnd(const char *ptr) const;
- _SQUID_INLINE_ bool willFit(const size_type toAppend) const;
- const InstanceId<MemBlob> id; ///< blob identifier
-
+ size_type calcAllocSize(const size_type minSize) const;
+
+ /// whether the offset points to the end of the used area
+ bool isAppendOffset(const size_type off) const { return off == size; }
+
+ /// whether n more bytes can be appended
+ bool willFit(const size_type n) const { return n <= spaceSize(); }
+
+ /* copying is not implemented */
+ MemBlob(const MemBlob &);
+ MemBlob& operator =(const MemBlob &);
};
MEMPROXY_CLASS_INLINE(MemBlob);
-#if _USE_INLINE_
-#include "MemBlob.cci"
-#endif
-
#endif /* SQUID_MEMBLOB_H_ */
