Detach libsbuf from StatHist to facilitate SBuf reuse (#1003)

The stated purpose of sbuf/DetailedStats was to "avoid adding external
dependencies to the SBuf code". DetailedStats failed to accomplish that
because it introduced a dependency on StatHist (which depends on Store).

To break the unwanted dependency, we outsource at-destruction-time size
statistics collection to external-to-SBuf code, configurable via the new
SBufStats fields. This also allows to avoid non-trivial SBuf statistics
collection in programs that do not need that statistics.

Since the new SBufStats fields are set at cache manager configuration
time in mainInitialize(), earlier SBuf and MemBlob destructions are not
accounted for. With more code changes/complications, we could initialize
the fields much earlier, but this delay may be considered a _positive_
change because it removes unusual SBuf stats from long-term histograms.

Also do not use the SBuf STUB in the deployed/non-test pinger program.
TODO: Convert more helpers to use SBuf. This change only adjusts pinger
because that adjustment did not require other significant changes.

diff --git a/src/Makefile.am b/src/Makefile.am
index 27bbff6ff50bc5090f74b649fff02d1caa252b0b..f6bf1f8f810edf080f35921dc0f02c82a0ae3c6b 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -929,7 +929,6 @@ tests_testLookupTable_SOURCES = \
        tests/testLookupTable.cc \
        tests/testLookupTable.h
 nodist_tests_testLookupTable_SOURCES = \
-       tests/stub_SBufDetailedStats.cc \
        base/LookupTable.h \
        tests/stub_debug.cc \
        tests/stub_libmem.cc
@@ -1143,7 +1142,6 @@ tests_testRock_SOURCES = \
        ResolvedPeers.h \
        tests/testRock.cc \
        tests/testRock.h \
-       tests/stub_SBufDetailedStats.cc \
        StatCounters.cc \
        StatCounters.h \
        tests/stub_StatHist.cc \
@@ -1315,7 +1313,6 @@ tests_testUfs_SOURCES = \
        RemovalPolicy.cc \
        RequestFlags.cc \
        RequestFlags.h \
-       tests/stub_SBufDetailedStats.cc \
        StatCounters.cc \
        StatCounters.h \
        StatHist.cc \
@@ -1494,7 +1491,6 @@ tests_testStore_SOURCES = \
        RemovalPolicy.cc \
        RequestFlags.cc \
        RequestFlags.h \
-       tests/stub_SBufDetailedStats.cc \
        StatCounters.cc \
        StatCounters.h \
        StatHist.cc \
@@ -1666,7 +1662,6 @@ tests_testDiskIO_SOURCES = \
        RequestFlags.h \
        ResolvedPeers.cc \
        ResolvedPeers.h \
-       tests/stub_SBufDetailedStats.cc \
        StatCounters.cc \
        StatCounters.h \
        tests/stub_StatHist.cc \
@@ -1916,7 +1911,6 @@ tests_test_http_range_SOURCES = \
        RequestFlags.h \
        ResolvedPeers.cc \
        ResolvedPeers.h \
-       tests/stub_SBufDetailedStats.cc \
        SquidMath.cc \
        SquidMath.h \
        StatCounters.cc \
@@ -2092,7 +2086,6 @@ tests_testHttp1Parser_SOURCES = \
        MemBuf.cc \
        MemBuf.h \
        tests/stub_MemObject.cc \
-       tests/stub_SBufDetailedStats.cc \
        String.cc \
        tests/stub_cache_cf.cc \
        cache_cf.h \
@@ -2167,7 +2160,6 @@ tests_testHttpReply_SOURCES = \
        MemBuf.h \
        Notes.cc \
        Notes.h \
-       tests/stub_SBufDetailedStats.cc \
        SquidString.h \
        SquidTime.h \
        StatCounters.cc \
@@ -2310,7 +2302,6 @@ tests_testHttpRequest_SOURCES = \
        RequestFlags.h \
        ResolvedPeers.cc \
        ResolvedPeers.h \
-       tests/stub_SBufDetailedStats.cc \
        SquidMath.cc \
        SquidMath.h \
        StatCounters.cc \
@@ -2617,7 +2608,6 @@ tests_testCacheManager_SOURCES = \
        RequestFlags.h \
        ResolvedPeers.cc \
        ResolvedPeers.h \
-       tests/stub_SBufDetailedStats.cc \
        SquidMath.cc \
        SquidMath.h \
        StatCounters.cc \
@@ -2791,7 +2781,6 @@ tests_testStatHist_SOURCES = \
        tests/stub_HelperChildConfig.cc \
        tests/stub_MemBuf.cc \
        tests/stub_MemObject.cc \
-       tests/stub_SBufDetailedStats.cc \
        StatHist.cc \
        tests/testStatHist.cc \
        StatHist.h \

diff --git a/src/SBufStatsAction.cc b/src/SBufStatsAction.cc
index 379c6f8ea48157e750041ec28a6aad84316cc6a1..79a18c47dc7945188817767a0c2ea2503fede154 100644 (file)
--- a/src/SBufStatsAction.cc
+++ b/src/SBufStatsAction.cc
@@ -11,9 +11,47 @@
 #include "ipc/Messages.h"
 #include "ipc/TypedMsgHdr.h"
 #include "mgr/Registration.h"
-#include "sbuf/DetailedStats.h"
+#include "sbuf/Stats.h"
 #include "SBufStatsAction.h"
 
+/// creates a new size-at-destruct-time histogram
+static StatHist *
+makeDestructTimeHist() {
+    const auto stats = new StatHist;
+    stats->logInit(100, 30.0, 128000.0);
+    return stats;
+}
+
+/// the SBuf size-at-destruct-time histogram
+static StatHist &
+collectSBufDestructTimeStats()
+{
+    static auto stats = makeDestructTimeHist();
+    return *stats;
+}
+
+/// the MemBlob size-at-destruct-time histogram
+static StatHist &
+collectMemBlobDestructTimeStats()
+{
+    static auto stats = makeDestructTimeHist();
+    return *stats;
+}
+
+/// record the size an SBuf had when it was destructed
+static void
+recordSBufSizeAtDestruct(const size_t sz)
+{
+    collectSBufDestructTimeStats().count(static_cast<double>(sz));
+}
+
+/// record the size a MemBlob had when it was destructed
+static void
+recordMemBlobSizeAtDestruct(const size_t sz)
+{
+    collectMemBlobDestructTimeStats().count(static_cast<double>(sz));
+}
+
 SBufStatsAction::SBufStatsAction(const Mgr::CommandPointer &cmd_):
     Action(cmd_)
 { } //default constructor is OK for data member
@@ -85,6 +123,8 @@ SBufStatsAction::unpack(const Ipc::TypedMsgHdr& msg)
 void
 SBufStatsAction::RegisterWithCacheManager()
 {
+    SBufStats::MemBlobSizeAtDestructRecorder = &recordMemBlobSizeAtDestruct;
+    SBufStats::SBufSizeAtDestructRecorder = &recordSBufSizeAtDestruct;
     Mgr::RegisterAction("sbuf", "String-Buffer statistics", &SBufStatsAction::Create, 0, 1);
 }
 

diff --git a/src/icmp/Makefile.am b/src/icmp/Makefile.am
index 64bdcfd5abe2cc7c75eee43490e1ef1139770d38..e5fb6bd9fb43b0af17248a0fd0fae4471e3ea51b 100644 (file)
--- a/src/icmp/Makefile.am
+++ b/src/icmp/Makefile.am
@@ -41,7 +41,6 @@ COPIED_SOURCE= \
        tests/stub_fd.cc \
        tests/stub_HelperChildConfig.cc \
        tests/stub_libmem.cc \
-       tests/stub_SBuf.cc \
        tests/STUB.h \
        time.cc
 
@@ -60,6 +59,7 @@ pinger_LDFLAGS = $(LIBADD_DL)
 pinger_LDADD=\
        libicmpcore.la \
        $(top_builddir)/src/ip/libip.la \
+       $(top_builddir)/src/sbuf/libsbuf.la \
        $(top_builddir)/src/debug/libdebug.la \
        $(top_builddir)/src/base/libbase.la \
        $(COMPAT_LIB) \
@@ -101,9 +101,6 @@ tests/stub_fd.cc: $(top_srcdir)/src/tests/stub_fd.cc | tests
 tests/stub_libmem.cc: $(top_srcdir)/src/tests/stub_libmem.cc | tests
        cp $(top_srcdir)/src/tests/stub_libmem.cc $@
 
-tests/stub_SBuf.cc: $(top_srcdir)/src/tests/stub_SBuf.cc | tests
-       cp $(top_srcdir)/src/tests/stub_SBuf.cc $@
-
 tests/STUB.h: $(top_srcdir)/src/tests/STUB.h | tests
        cp $(top_srcdir)/src/tests/STUB.h $@
 

diff --git a/src/sbuf/DetailedStats.cc b/src/sbuf/DetailedStats.cc
deleted file mode 100644 (file)
index 9f0d3e9..0000000
--- a/src/sbuf/DetailedStats.cc
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * Copyright (C) 1996-2022 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-#include "squid.h"
-#include "sbuf/DetailedStats.h"
-#include "StatHist.h"
-
-/*
- * Implementation note: the purpose of this construct is to avoid adding
- * external dependencies to the SBuf code
- */
-
-static StatHist *
-newStatHist() {
-    StatHist *stats = new StatHist;
-    stats->logInit(100, 30.0, 128000.0);
-    return stats;
-}
-
-StatHist &
-collectSBufDestructTimeStats()
-{
-    static StatHist *stats = newStatHist();
-    return *stats;
-}
-
-StatHist &
-collectMemBlobDestructTimeStats()
-{
-    static StatHist *stats = newStatHist();
-    return *stats;
-}
-
-void
-recordSBufSizeAtDestruct(SBuf::size_type sz)
-{
-    collectSBufDestructTimeStats().count(static_cast<double>(sz));
-}
-
-void
-recordMemBlobSizeAtDestruct(SBuf::size_type sz)
-{
-    collectMemBlobDestructTimeStats().count(static_cast<double>(sz));
-}
-

diff --git a/src/sbuf/DetailedStats.h b/src/sbuf/DetailedStats.h
deleted file mode 100644 (file)
index 8d13c52..0000000
--- a/src/sbuf/DetailedStats.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/*
- * Copyright (C) 1996-2022 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-#ifndef SQUID_SBUFDETAILEDSTATS_H
-#define SQUID_SBUFDETAILEDSTATS_H
-
-#include "sbuf/SBuf.h"
-
-class StatHist;
-
-/// Record the size a SBuf had when it was destructed
-void recordSBufSizeAtDestruct(SBuf::size_type sz);
-
-/// the SBuf size-at-destruct-time histogram
-StatHist &collectSBufDestructTimeStats();
-
-/// Record the size a MemBlob had when it was destructed
-void recordMemBlobSizeAtDestruct(MemBlob::size_type sz);
-
-/// the MemBlob size-at-destruct-time histogram
-StatHist &collectMemBlobDestructTimeStats();
-
-#endif /* SQUID_SBUFDETAILEDSTATS_H */
-

diff --git a/src/sbuf/Makefile.am b/src/sbuf/Makefile.am
index 4b8c2f0e01d2a4623d818ae5dcfa7348ff04be2b..87415c9556cbe453729b7cce3a0d62f43d5baef5 100644 (file)
--- a/src/sbuf/Makefile.am
+++ b/src/sbuf/Makefile.am
@@ -13,8 +13,6 @@ noinst_LTLIBRARIES = libsbuf.la
 libsbuf_la_SOURCES = \
        Algorithms.cc \
        Algorithms.h \
-       DetailedStats.cc \
-       DetailedStats.h \
        List.cc \
        List.h \
        MemBlob.cc \

diff --git a/src/sbuf/MemBlob.cc b/src/sbuf/MemBlob.cc
index 7993f476dabf06a62d6ca0d08b55e819999cfb43..78a24dfd97a867fa3d5e97a21542a68ff7882133 100644 (file)
--- a/src/sbuf/MemBlob.cc
+++ b/src/sbuf/MemBlob.cc
@@ -9,8 +9,8 @@
 #include "squid.h"
 #include "base/TextException.h"
 #include "debug/Stream.h"
-#include "sbuf/DetailedStats.h"
 #include "sbuf/MemBlob.h"
+#include "sbuf/Stats.h"
 
 #include <iostream>
 
@@ -74,7 +74,7 @@ MemBlob::~MemBlob()
         memFreeString(capacity,mem);
     Stats.liveBytes -= capacity;
     --Stats.live;
-    recordMemBlobSizeAtDestruct(capacity);
+    SBufStats::RecordMemBlobSizeAtDestruct(capacity);
 
     debugs(MEMBLOB_DEBUGSECTION,9, "destructed, this="
            << static_cast<void*>(this) << " id=" << id

diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc
index af044c9ca0e8e51e01031dd5f1bc940aee70202c..e4286744eaf9b420721c93fc6dc06739eeb52537 100644 (file)
--- a/src/sbuf/SBuf.cc
+++ b/src/sbuf/SBuf.cc
@@ -11,7 +11,6 @@
 #include "base/Raw.h"
 #include "base/RefCount.h"
 #include "debug/Stream.h"
-#include "sbuf/DetailedStats.h"
 #include "sbuf/SBuf.h"
 #include "util.h"
 
@@ -70,7 +69,7 @@ SBuf::~SBuf()
 {
     debugs(24, 8, id << " destructed");
     --stats.live;
-    recordSBufSizeAtDestruct(len_);
+    SBufStats::RecordSBufSizeAtDestruct(len_);
 }
 
 MemBlob::Pointer

diff --git a/src/sbuf/Stats.cc b/src/sbuf/Stats.cc
index 0d884990cbb6a68f7ac2ea6f34fb37e0882c352e..b03021fb8cdccee8148682325eaf1b519f055452 100644 (file)
--- a/src/sbuf/Stats.cc
+++ b/src/sbuf/Stats.cc
@@ -13,6 +13,23 @@
 
 #include <iostream>
 
+SBufStats::SizeRecorder SBufStats::SBufSizeAtDestructRecorder = nullptr;
+SBufStats::SizeRecorder SBufStats::MemBlobSizeAtDestructRecorder = nullptr;
+
+void
+SBufStats::RecordSBufSizeAtDestruct(const size_t sz)
+{
+    if (SBufSizeAtDestructRecorder)
+        SBufSizeAtDestructRecorder(sz);
+}
+
+void
+SBufStats::RecordMemBlobSizeAtDestruct(const size_t sz)
+{
+    if (MemBlobSizeAtDestructRecorder)
+        MemBlobSizeAtDestructRecorder(sz);
+}
+
 SBufStats&
 SBufStats::operator +=(const SBufStats& ss)
 {

diff --git a/src/sbuf/Stats.h b/src/sbuf/Stats.h
index 833decd8ef86240bf94b3c1bec14c21eb1200c0c..defb68ab72570d483ee112af0ae4e469230aae9f 100644 (file)
--- a/src/sbuf/Stats.h
+++ b/src/sbuf/Stats.h
@@ -26,6 +26,12 @@ public:
 
     SBufStats& operator +=(const SBufStats&);
 
+    /// Record the size a SBuf had when it was destructed
+    static void RecordSBufSizeAtDestruct(size_t);
+
+    /// Record the size a MemBlob had when it was destructed
+    static void RecordMemBlobSizeAtDestruct(size_t);
+
 public:
     uint64_t alloc = 0; ///<number of calls to SBuf constructors
     uint64_t allocCopy = 0; ///<number of calls to SBuf copy-constructor
@@ -51,6 +57,13 @@ public:
     uint64_t cowJustAlloc = 0; ///< number of cow() calls requiring just a new empty buffer
     uint64_t cowAllocCopy = 0; ///< number of cow() calls requiring copying into a new buffer
     uint64_t live = 0;  ///<number of currently-allocated SBuf
+
+    /// function for collecting detailed size-related statistics
+    using SizeRecorder = void (*)(size_t);
+    /// collects statistics about SBuf sizes at SBuf destruction time
+    static SizeRecorder SBufSizeAtDestructRecorder;
+    /// collects statistics about MemBlob capacity at MemBlob destruction time
+    static SizeRecorder MemBlobSizeAtDestructRecorder;
 };
 
 #endif /* SQUID_SBUF_STATS_H */

diff --git a/src/tests/Stub.am b/src/tests/Stub.am
index 6585cafb64e8d84737f19f2b2874347cc8ae1474..a7e133f90699bf7905dc80f096f95413fcb7e153 100644 (file)
--- a/src/tests/Stub.am
+++ b/src/tests/Stub.am
@@ -75,7 +75,6 @@ STUB_SOURCE = \
     tests/stub_Port.cc \
     tests/stub_redirect.cc \
     tests/stub_SBuf.cc \
-    tests/stub_SBufDetailedStats.cc \
     tests/stub_stat.cc \
     tests/stub_StatHist.cc \
     tests/stub_stmem.cc \

diff --git a/src/tests/stub_SBuf.cc b/src/tests/stub_SBuf.cc
index f0d30a7570a89df5a171ad4bb34a12c25bd0112e..6dc70071f5632ab915c1c7d83de74d83461802d2 100644 (file)
--- a/src/tests/stub_SBuf.cc
+++ b/src/tests/stub_SBuf.cc
@@ -19,6 +19,8 @@ SBufStats SBuf::stats;
 const SBuf::size_type SBuf::npos;
 const SBuf::size_type SBuf::maxSize;
 
+SBufStats::SizeRecorder SBufStats::SBufSizeAtDestructRecorder = nullptr;
+SBufStats::SizeRecorder SBufStats::MemBlobSizeAtDestructRecorder = nullptr;
 std::ostream& SBufStats::dump(std::ostream &os) const STUB_RETVAL(os)
 SBufStats& SBufStats::operator +=(const SBufStats&) STUB_RETVAL(*this)
 

diff --git a/src/tests/stub_SBufDetailedStats.cc b/src/tests/stub_SBufDetailedStats.cc
deleted file mode 100644 (file)
index 42080c9..0000000
--- a/src/tests/stub_SBufDetailedStats.cc
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Copyright (C) 1996-2022 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-#include "squid.h"
-#include "sbuf/DetailedStats.h"
-#include "sbuf/SBuf.h"
-#include "StatHist.h"
-
-#define STUB_API "sbuf/DetailedStats.cc"
-#include "tests/STUB.h"
-
-static StatHist s;
-
-void recordSBufSizeAtDestruct(SBuf::size_type) {} // STUB_NOP
-StatHist &collectSBufDestructTimeStats() STUB_RETVAL(s)
-void recordMemBlobSizeAtDestruct(SBuf::size_type) {} // STUB_NOP
-StatHist &collectMemBlobDestructTimeStats() STUB_RETVAL(s)
-