From f079ed87b200bc8e148a1a1b266760402e3aeb17 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 25 Mar 2022 16:17:54 +0000 Subject: [PATCH] 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. --- src/Makefile.am | 11 ------- src/SBufStatsAction.cc | 42 +++++++++++++++++++++++- src/icmp/Makefile.am | 5 +-- src/sbuf/DetailedStats.cc | 50 ----------------------------- src/sbuf/DetailedStats.h | 29 ----------------- src/sbuf/Makefile.am | 2 -- src/sbuf/MemBlob.cc | 4 +-- src/sbuf/SBuf.cc | 3 +- src/sbuf/Stats.cc | 17 ++++++++++ src/sbuf/Stats.h | 13 ++++++++ src/tests/Stub.am | 1 - src/tests/stub_SBuf.cc | 2 ++ src/tests/stub_SBufDetailedStats.cc | 23 ------------- 13 files changed, 77 insertions(+), 125 deletions(-) delete mode 100644 src/sbuf/DetailedStats.cc delete mode 100644 src/sbuf/DetailedStats.h delete mode 100644 src/tests/stub_SBufDetailedStats.cc diff --git a/src/Makefile.am b/src/Makefile.am index 27bbff6ff5..f6bf1f8f81 100644 --- 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 379c6f8ea4..79a18c47dc 100644 --- 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(sz)); +} + +/// record the size a MemBlob had when it was destructed +static void +recordMemBlobSizeAtDestruct(const size_t sz) +{ + collectMemBlobDestructTimeStats().count(static_cast(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 64bdcfd5ab..e5fb6bd9fb 100644 --- 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 index 9f0d3e9899..0000000000 --- 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(sz)); -} - -void -recordMemBlobSizeAtDestruct(SBuf::size_type sz) -{ - collectMemBlobDestructTimeStats().count(static_cast(sz)); -} - diff --git a/src/sbuf/DetailedStats.h b/src/sbuf/DetailedStats.h deleted file mode 100644 index 8d13c529d0..0000000000 --- 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 4b8c2f0e01..87415c9556 100644 --- 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 7993f476da..78a24dfd97 100644 --- 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 @@ -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(this) << " id=" << id diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc index af044c9ca0..e4286744ea 100644 --- 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 0d884990cb..b03021fb8c 100644 --- a/src/sbuf/Stats.cc +++ b/src/sbuf/Stats.cc @@ -13,6 +13,23 @@ #include +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 833decd8ef..defb68ab72 100644 --- 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; ///