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 #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(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(this) - << ", reserveSize=" << reserveSize); + debugs(MEMBLOB_DEBUGSECTION,9, HERE << "constructed, this=" + << static_cast(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(this) - << ", memBlock=" << static_cast(memBlock) - << ", memSize=" << memSize); - memAlloc(memSize); - append(memBlock,memSize); - + debugs(MEMBLOB_DEBUGSECTION,9, HERE << "constructed, this=" + << static_cast(this) << " id=" << id << + << " buffer=" << static_cast(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(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(memAllocString(minSize, &actualAlloc)); #else - //\todo reinstate mempools use - mem=static_cast(xmalloc(actualAlloc)); + // \todo reinstate mempools use + mem = static_cast(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(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 - * - * 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; /// 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 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_ */