On Tue, Jan 3, 2012 at 3:32 AM, Amos Jeffries <[email protected]> wrote: > On 3/01/2012 10:11 a.m., Kinkie wrote: >> >> Hi all, >> here's a new interim merge proposal for the StatHist refactoring >> effort. This merge aims to close bug 3381 and advance a bit the >> general StatHist refactoring effort. >> >> What it does: >> - change the type used for StatHist counters to uint64_t (parametric >> as StatHist::bins_type) >> - change the index for StatHist bins to unsigned >> - bring stub_StatHist forward and actually use it, also to remove some >> objects from tests >> >> What crept in: >> - an initial effort for a StatHist unit test. >> - some src/Makefile.am cleanup >> >> > > > in StatHist.cc: > * StatHist::findBin() - float type is quite inaccurate. Can you use double?
Yes. Thanks > * StatHist::operator =() - rather than duplicating this code can you make > it inline? I'm not sure I understand what you mean. Please keep in mind that this code is currently in due to the need to satisfy the squid3 mandatory coding guidelines (StatHist has default constructor and destructor, so it must have assignment). Eventually I plan to evolve StatHist to a class hierarchy, with: StatHistBase: responsible for memory management StatHistLog, StatHistEnum, StatHistInt: derived from StatHistBase, each responsible for a different flavor of counting, thus dismissing the explicit init methods > in testStatHist: > * please define testStatHistEnum() or replace with a TODO. At the cost of a bit of further creep-in, done. Here's a revised patch, which on top of the previous one, also: - actually uses the unit test - delivers a complete default constructor for StatHist - adds equality test for StatHist - adds more stubs to the collection (stub_mem and stub_stmem) and uses them -- /kinkie
# Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: [email protected] # target_branch: ../squid-trunk-co/ # testament_sha1: 4902210c1a088218b2e1dcb55f1d586cb0bb9671 # timestamp: 2012-01-03 16:17:44 +0100 # source_branch: lp:~kinkie/squid/stathist # base_revision_id: [email protected]\ # ruf92ac9g0q7ucid # # Begin patch === modified file 'src/Makefile.am' --- src/Makefile.am 2012-01-03 02:19:30 +0000 +++ src/Makefile.am 2012-01-03 15:17:09 +0000 @@ -1015,6 +1015,7 @@ tests/testString \ tests/testURL \ tests/testConfigParser \ + tests/testStatHist \ $(STORE_TESTS) ## NP: required to run the above list. check_PROGRAMS only builds the binaries... @@ -1114,27 +1115,7 @@ $(XTRA_LIBS) tests_testHttpReply_DEPENDENCIES= $(SQUID_CPPUNIT_LA) -## Tests for the ACLMaxUserIP class -## acl needs wordlist. wordlist needs MemBug -## MemBuf needs mem, MemBuf needs event, -## event needs cbdata. -## ACLMaxUserUP needs $(AUTH_LIBS) -## ACLMaxUserIP needs ACLChecklist -## AuthUser request needs HttpHeader, which brings in -## ETag.cc \ -## HttpHeader.cc \ -## HttpHeaderTools.cc \ -## HttpHdrContRange.cc \ -## HttpHdrCc.cc \ -## HttpHdrRange.cc \ -## HttpHdrSc.cc \ -## HttpHdrScTarget.cc \ -## Packer.cc \ -## StatHist.cc \ -## String.cc \ -## -## disk.cc \ -## fs/libfs.la \ + tests_testACLMaxUserIP_SOURCES= \ cbdata.cc \ ClientInfo.h \ @@ -1167,7 +1148,7 @@ SquidMath.cc \ StatCounters.h \ StatHist.h \ - StatHist.cc \ + tests/stub_StatHist.cc \ stmem.cc \ String.cc \ store_dir.cc \ @@ -1369,7 +1350,7 @@ StatCounters.h \ StatCounters.cc \ StatHist.h \ - StatHist.cc \ + tests/stub_StatHist.cc \ stmem.cc \ store.cc \ store_client.cc \ @@ -1495,7 +1476,7 @@ StatCounters.h \ StatCounters.cc \ StatHist.h \ - StatHist.cc \ + tests/stub_StatHist.cc \ stmem.cc \ StoreFileSystem.cc \ StoreIOState.cc \ @@ -2764,7 +2745,7 @@ StatCounters.h \ StatCounters.cc \ StatHist.h \ - StatHist.cc \ + tests/stub_StatHist.cc \ stmem.cc \ store.cc \ StoreFileSystem.cc \ @@ -2935,9 +2916,7 @@ StatCounters.h \ StatCounters.cc \ StatHist.h \ - StatHist.cc \ - HttpHdrRange.cc \ - ETag.cc \ + tests/stub_StatHist.cc \ tests/stub_errorpage.cc \ tests/stub_HttpRequest.cc \ tests/stub_access_log.cc \ @@ -3073,9 +3052,7 @@ StatCounters.h \ StatCounters.cc \ StatHist.h \ - StatHist.cc \ - HttpHdrRange.cc \ - ETag.cc \ + tests/stub_StatHist.cc \ tests/stub_errorpage.cc \ tests/stub_HttpRequest.cc \ tests/stub_access_log.cc \ @@ -3227,7 +3204,7 @@ StatCounters.h \ StatCounters.cc \ StatHist.h \ - StatHist.cc \ + tests/stub_StatHist.cc \ stmem.cc \ store.cc \ store_client.cc \ @@ -3347,6 +3324,40 @@ tests_testConfigParser_LDFLAGS = $(LIBADD_DL) tests_testConfigParser_DEPENDENCIES = \ $(SQUID_CPPUNIT_LA) + +tests_testStatHist_SOURCES = \ + tests/testStatHist.h \ + tests/testStatHist.cc \ + tests/testMain.cc \ + StatHist.h \ + StatHist.cc \ + String.cc \ + tests/stub_store.cc \ + tests/stub_store_stats.cc \ + tests/stub_MemObject.cc \ + tests/stub_stmem.cc \ + tests/stub_debug.cc \ + tests/stub_comm.cc \ + cbdata.cc \ + tests/stub_DelayId.cc \ + tests/stub_mem.cc \ + time.cc \ + MemBuf.cc \ + tests/stub_cache_manager.cc \ + tests/stub_pconn.cc \ + tests/stub_HelperChildConfig.cc \ + tests/stub_mime.cc +nodist_tests_testStatHist_SOURCES = \ + $(TESTSOURCES) +tests_testStatHist_LDFLAGS = $(LIBADD_DL) +tests_testStatHist_LDADD = \ + base/libbase.la \ + $(SQUID_CPPUNIT_LIBS) \ + $(SQUID_CPPUNIT_LA) \ + $(COMPAT_LIB) \ + $(top_builddir)/lib/libmiscutil.la +tests_testStatHist_DEPENDENCIES = $(SQUID_CPPUNIT_LA) + TESTS += testHeaders === modified file 'src/StatHist.cc' --- src/StatHist.cc 2011-12-18 01:31:58 +0000 +++ src/StatHist.cc 2012-01-03 15:06:10 +0000 @@ -46,9 +46,8 @@ /* low level init, higher level functions has less params */ void -StatHist::init(int newCapacity, hbase_f * val_in_, hbase_f * val_out_, double newMin, double newMax) +StatHist::init(unsigned int newCapacity, hbase_f * val_in_, hbase_f * val_out_, double newMin, double newMax) { - assert(newCapacity > 0); assert(val_in_ && val_out_); /* check before we divide to get scale_ */ assert(val_in_(newMax - newMin) > 0); @@ -57,22 +56,22 @@ capacity_ = newCapacity; val_in = val_in_; val_out = val_out_; - bins = static_cast<int *>(xcalloc(capacity_, sizeof(int))); + bins = static_cast<bins_type *>(xcalloc(capacity_, sizeof(bins_type))); scale_ = capacity_ / val_in(max_ - min_); /* check that functions are valid */ /* a min value should go into bin[0] */ - assert(findBin(min_) == 0); + assert(findBin(min_) == 0); //TODO: move to unit test /* a max value should go into the last bin */ - assert(findBin(max_) == capacity_ - 1); + assert(findBin(max_) == capacity_ - 1); //TODO: move to unit test /* it is hard to test val_out, here is a crude test */ - assert(((int) floor(0.99 + val(0) - min_)) == 0); + assert(((int) floor(0.99 + val(0) - min_)) == 0); //TODO: move to unit test } void StatHist::clear() { - for (int i=0; i<capacity_; ++i) + for (unsigned int i=0; i<capacity_; ++i) bins[i]=0; } @@ -84,19 +83,16 @@ } } +// any changes must be copied to stub_StatHist StatHist& StatHist::operator =(const StatHist & src) { if (this==&src) //handle self-assignment return *this; - assert(src.bins != NULL); // TODO: remove after initializing bins at construction time - if (capacity_ != src.capacity_) { - // need to resize. + if (bins != NULL) xfree(bins); - bins = static_cast<int *>(xcalloc(src.capacity_, sizeof(int))); - capacity_=src.capacity_; - - } + capacity_=src.capacity_; + bins = static_cast<bins_type *>(xcalloc(src.capacity_, sizeof(bins_type))); min_=src.min_; max_=src.max_; scale_=src.scale_; @@ -106,39 +102,60 @@ return *this; } + +bool +StatHist::operator ==(const StatHist & src) +{ + assert(bins != NULL && src.bins != NULL); // TODO: remove after initializing bins at construction time + if (capacity_ != src.capacity_ || + min_!=src.min_ || + max_!=src.max_ || + scale_!=src.scale_ || + val_in!=src.val_in || + val_out!=src.val_out) + return false; + return (memcmp(bins,src.bins,capacity_*sizeof(*bins))==0); +} + StatHist::StatHist(const StatHist &src) : capacity_(src.capacity_), min_(src.min_), max_(src.max_), scale_(src.scale_), val_in(src.val_in), val_out(src.val_out) { if (src.bins!=NULL) { - bins = static_cast<int *>(xcalloc(src.capacity_, sizeof(int))); + bins = static_cast<bins_type *>(xcalloc(src.capacity_, sizeof(int))); memcpy(bins,src.bins,capacity_*sizeof(*bins)); } } +StatHist::StatHist() : + bins(NULL), capacity_(0), min_(0), max_(0), + scale_(1.0), val_in(NULL), val_out(NULL) +{} + void StatHist::count(double val) { - const int bin = findBin(val); + const unsigned int bin = findBin(val); assert(bins); /* make sure it got initialized */ - assert(0 <= bin && bin < capacity_); + assert(bin < capacity_); ++bins[bin]; } -int +unsigned int StatHist::findBin(double v) { - int bin; v -= min_; /* offset */ if (v <= 0.0) /* too small */ return 0; - bin = (int) floor(scale_ * val_in(v) + 0.5); + unsigned int bin; + double tmp_bin=floor(scale_ * val_in(v) + 0.5); - if (bin < 0) /* should not happen */ + if (tmp_bin < 0.0) // should not happen return 0; + bin = static_cast <unsigned int>(tmp_bin); if (bin >= capacity_) /* too big */ bin = capacity_ - 1; @@ -147,7 +164,7 @@ } double -StatHist::val(int bin) const +StatHist::val(unsigned int bin) const { return val_out((double) bin / scale_) + min_; } @@ -167,14 +184,14 @@ double StatHist::deltaPctile(const StatHist & B, double pctile) const { - int i; - int s1 = 0; - int h = 0; - int a = 0; - int b = 0; - int I = 0; - int J = capacity_; - int K; + unsigned int i; + bins_type s1 = 0; + bins_type h = 0; + bins_type a = 0; + bins_type b = 0; + unsigned int I = 0; + unsigned int J = capacity_; + unsigned int K; double f; assert(capacity_ == B.capacity_); @@ -219,7 +236,7 @@ f = (h - a) / (b - a); - K = (int) floor(f * (double) (J - I) + I); + K = (unsigned int) floor(f * (double) (J - I) + I); return val(K); } @@ -235,13 +252,12 @@ void StatHist::dump(StoreEntry * sentry, StatHistBinDumper * bd) const { - int i; double left_border = min_; if (!bd) bd = statHistBinDumper; - for (i = 0; i < capacity_; ++i) { + for (unsigned int i = 0; i < capacity_; ++i) { const double right_border = val(i + 1); assert(right_border - left_border > 0.0); bd(sentry, i, left_border, right_border - left_border, bins[i]); @@ -264,7 +280,7 @@ } void -StatHist::logInit(int capacity, double min, double max) +StatHist::logInit(unsigned int capacity, double min, double max) { init(capacity, Math::Log, Math::Exp, min, max); } @@ -278,7 +294,7 @@ } void -StatHist::enumInit(int last_enum) +StatHist::enumInit(unsigned int last_enum) { init(last_enum + 3, Math::Null, Math::Null, -1.0, (2.0 + last_enum)); } === modified file 'src/StatHist.h' --- src/StatHist.h 2011-12-17 21:23:14 +0000 +++ src/StatHist.h 2012-01-03 15:06:10 +0000 @@ -55,11 +55,15 @@ * \todo specialize the class in a small hierarchy so that all * relevant initializations are done at build-time */ - StatHist() : scale_(1.0) {} + StatHist(); StatHist(const StatHist&); //not needed ~StatHist(); + typedef uint64_t bins_type; + StatHist &operator=(const StatHist &); + bool operator==(const StatHist &); + /** clear the contents of the histograms * * \todo remove: this function has been replaced in its purpose @@ -74,7 +78,7 @@ /** obtain the output-transformed value from the specified bin * */ - double val(int bin) const; + double val(unsigned int bin) const; /** increment the counter for the histogram entry * associated to the supplied value */ @@ -84,10 +88,10 @@ void dump(StoreEntry *sentry, StatHistBinDumper * bd) const; /** Initialize the Histogram using a logarithmic values distribution */ - void logInit(int capacity, double min, double max); + void logInit(unsigned int capacity, double min, double max); /** initialize the histogram to count occurrences in an enum-represented set */ - void enumInit(int last_enum); + void enumInit(unsigned int last_enum); protected: /** low-level initialize function. called by *Init high-level functions * \note Important restrictions on val_in and val_out functions: @@ -100,13 +104,13 @@ * val_in is applied after offseting the value but before scaling * See log and linear based histograms for examples */ - void init(int capacity, hbase_f * val_in, hbase_f * val_out, double min, double max); + void init(unsigned int capacity, hbase_f * val_in, hbase_f * val_out, double min, double max); /// find what entry in the histogram corresponds to v, by applying /// the preset input transformation function - int findBin(double v); + unsigned int findBin(double v); /// the histogram counters - int *bins; - int capacity_; + bins_type *bins; + unsigned int capacity_; /// minimum value to be stored, corresponding to the first bin double min_; /// value of the maximum counter in the histogram === modified file 'src/tests/stub_StatHist.cc' --- src/tests/stub_StatHist.cc 2011-12-15 07:01:07 +0000 +++ src/tests/stub_StatHist.cc 2012-01-03 15:06:10 +0000 @@ -1,24 +1,71 @@ #include "config.h" +#define STUB_API "StatHist.cc" #include "STUB.h" #include "StatHist.h" -#define STUB_API "StatHist.cc" - -void -StatHist::init(int capacity_, hbase_f * val_in_, hbase_f * val_out_, double min_, double max_) -{} StatHist::~StatHist() {} void -StatHist::enumInit(int last_enum) -{} - -void -StatHist::count(double val) -{} - -void StatHist::dump(StoreEntry * sentry, StatHistBinDumper * bd) const {} + +void +StatHist::enumInit(unsigned int i) +{} + +void +StatHist::count(double d) +{} + +double +statHistDeltaMedian(const StatHist & A, const StatHist & B) +STUB_RETVAL(0.0) + +double +statHistDeltaPctile(const StatHist & A, const StatHist & B, double pctile) +STUB_RETVAL(0.0) + +void +StatHist::clear() +STUB + +void +StatHist::logInit(unsigned int i, double d1, double d2) +STUB + +//verbatim copy from StatHist.cc +StatHist::StatHist() : + bins(NULL), capacity_(0), min_(0), max_(0), + scale_(1.0), val_in(NULL), val_out(NULL) +{} + +//verbatim copy from StatHist.cc +StatHist& +StatHist::operator =(const StatHist & src) +{ + if (this==&src) //handle self-assignment + return *this; + assert(src.bins != NULL); // TODO: remove after initializing bins at construction time + if (capacity_ != src.capacity_) { + // need to resize. + xfree(bins); + bins = static_cast<bins_type *>(xcalloc(src.capacity_, sizeof(bins_type))); + capacity_=src.capacity_; + + } + min_=src.min_; + max_=src.max_; + scale_=src.scale_; + val_in=src.val_in; + val_out=src.val_out; + memcpy(bins,src.bins,capacity_*sizeof(*bins)); + return *this; +} + +class StoreEntry; +void +statHistIntDumper(StoreEntry * sentry, int idx, double val, double size, int count) +STUB + === added file 'src/tests/stub_mem.cc' --- src/tests/stub_mem.cc 1970-01-01 00:00:00 +0000 +++ src/tests/stub_mem.cc 2012-01-03 15:06:10 +0000 @@ -0,0 +1,63 @@ +/* + * stub file for mem.cc + */ + +#include "config.h" + +#define STUB_API "stub_mem.cc" +#include "STUB.h" +#include "compat/xalloc.h" +#include "typedefs.h" /* for FREE */ + +extern "C" void +memFreeString(size_t size, void *buf) +{ + xfree(buf); +} + +extern "C" void * +memAllocString(size_t net_size, size_t * gross_size) +{ + *gross_size=net_size; + return xmalloc(net_size); +} + +extern "C" void +memFreeBuf(size_t size, void *buf) +{ + xfree(buf); +} + +extern "C" void * +memAllocBuf(size_t net_size, size_t * gross_size) +{ + *gross_size=net_size; + return xcalloc(1, net_size); +} + +/* net_size is the new size, *gross size is the old gross size, to be changed to + * the new gross size as a side-effect. + */ +extern "C" void * +memReallocBuf(void *oldbuf, size_t net_size, size_t * gross_size) +{ + void *rv=xrealloc(oldbuf,net_size); +// if (net_size > *gross_size) +// memset(rv+net_size,0,net_size-*gross_size); + *gross_size=net_size; + return rv; +} + +FREE * +memFreeBufFunc(size_t size) +{ + return xfree; +} + +void * +memAllocate(mem_type type) +STUB_RETVAL(NULL) + +void +memFree(void *p, int type) +STUB === modified file 'src/tests/stub_stmem.cc' --- src/tests/stub_stmem.cc 2009-01-21 03:47:47 +0000 +++ src/tests/stub_stmem.cc 2012-01-03 15:06:10 +0000 @@ -32,8 +32,11 @@ * */ -#include "squid.h" +#include "config.h" +#define STUB_API "mem_hdr" +#include "tests/STUB.h" #include "stmem.h" +#include "StoreIOBuffer.h" mem_hdr::mem_hdr() {} @@ -43,7 +46,12 @@ size_t mem_hdr::size() const -{ - fatal ("Not implemented"); - return 0; -} +STUB_RETVAL(0) + +int64_t +mem_hdr::endOffset () const +STUB_RETVAL(0) + +bool +mem_hdr::write (StoreIOBuffer const &writeBuffer) +STUB_RETVAL(false) === added file 'src/tests/testStatHist.cc' --- src/tests/testStatHist.cc 1970-01-01 00:00:00 +0000 +++ src/tests/testStatHist.cc 2012-01-03 15:06:10 +0000 @@ -0,0 +1,50 @@ +#define SQUID_UNIT_TEST 1 +#include "config.h" +#include "testStatHist.h" +#include "StatHist.h" + +CPPUNIT_TEST_SUITE_REGISTRATION(testStatHist); + +typedef enum { + ZERO, ONE, TWO, THREE, FOUR, FIVE +} number ; + +void +testStatHist::testStatHistBaseEquality() +{ + StatHist raw, test; + raw.enumInit(FIVE); + test.enumInit(FIVE); + CPPUNIT_ASSERT(raw==test); + test.count(ZERO); + CPPUNIT_ASSERT_ASSERTION_FAIL(CPPUNIT_ASSERT(raw==test)); +} + +void +testStatHist::testStatHistBaseAssignment() +{ + StatHist raw, test; + raw.enumInit(FIVE); + test.enumInit(FIVE); + test.count(ZERO); + CPPUNIT_ASSERT_ASSERTION_FAIL(CPPUNIT_ASSERT(raw==test)); + test=raw; + CPPUNIT_ASSERT(raw==test); +} + + +void +testStatHist::testStatHistLog() +{ + const double min=0.0, max=10000.0; + const int capacity=10; + StatHist raw, test; + raw.logInit(capacity,min,max); + test=raw; + test.count(min); + //CPPUNIT_ASSERT(test.val(0)==1); //FIXME: val() returns a density + //CPPUNIT_ASSERT(test.val(1)==0); //FIXME: val() returns a density + test=raw; + test.count(max); + //CPPUNIT_ASSERT(test.val(capacity-1)==1); //FIXME: val() returns a density +} === added file 'src/tests/testStatHist.h' --- src/tests/testStatHist.h 1970-01-01 00:00:00 +0000 +++ src/tests/testStatHist.h 2012-01-03 15:06:10 +0000 @@ -0,0 +1,27 @@ +/* + * StatHist unit test + */ + +#ifndef TESTSTATHIST_H_ +#define TESTSTATHIST_H_ + +#include <cppunit/extensions/HelperMacros.h> + + +class testStatHist : public CPPUNIT_NS::TestFixture +{ + CPPUNIT_TEST_SUITE( testStatHist ); + CPPUNIT_TEST( testStatHistBaseEquality ); + CPPUNIT_TEST( testStatHistBaseAssignment ); + CPPUNIT_TEST( testStatHistLog ); + CPPUNIT_TEST_SUITE_END(); + + public: + + protected: + void testStatHistBaseEquality(); + void testStatHistBaseAssignment(); + void testStatHistLog(); +}; + +#endif /* TESTSTATHIST_H_ */
