From bb6401aaf32952307befc749f581c16ec6eeee77 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sun, 8 Feb 2026 23:44:17 +0100 Subject: [PATCH] BufferProvider: reshape diagnostics into a decorator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The intention is to build the tracking functionality from scratch, while wrapping the old implementation as delegate. Hopefully this allows to reduce the complexity of the existing implementation step by step, until I'm ''able to grasp what this implementation does''. This old implementation works, but is highly problematic code. The Git history shows that I had already problems to understand this code while building the DiagnosticOutputSlot, one year later. To my defense, in both cases (2011 and 2012), I had no clear idea yet how the processing within the engine would work, and thus I considered that Buffer Type registry a mere implementation detail, that can be lumped into the »code behind«. --- .../engine/diagnostic-buffer-provider.cpp | 67 ++++----------- .../engine/diagnostic-buffer-provider.hpp | 82 +++++++++---------- src/steam/engine/heap-mem-provider.cpp | 8 +- src/steam/engine/heap-mem-provider.hpp | 2 +- .../engine/buffer-provider-protocol-test.cpp | 24 +++--- tests/core/steam/engine/node-feed-test.cpp | 8 +- wiki/thinkPad.ichthyo.mm | 70 +++++++++++++--- 7 files changed, 130 insertions(+), 131 deletions(-) diff --git a/src/steam/engine/diagnostic-buffer-provider.cpp b/src/steam/engine/diagnostic-buffer-provider.cpp index 7037cd910..d809bd5b5 100644 --- a/src/steam/engine/diagnostic-buffer-provider.cpp +++ b/src/steam/engine/diagnostic-buffer-provider.cpp @@ -20,8 +20,9 @@ */ -#include "lib/error.hpp" -#include "lib/meta/function.hpp" +//#include "lib/error.hpp" +//#include "include/logging.h" +//#include "lib/meta/function.hpp" #include "steam/engine/diagnostic-buffer-provider.hpp" #include "steam/engine/heap-mem-provider.hpp" @@ -31,54 +32,14 @@ namespace steam { namespace engine { - /** Storage for the diagnostics frontend */ - lib::Depend DiagnosticBufferProvider::diagnostics; - - - - DiagnosticBufferProvider::DiagnosticBufferProvider() - : pImpl_() + : BufferProvider{"Diagnostic_HeapAllocated"} { } - DiagnosticBufferProvider::~DiagnosticBufferProvider() { } - - - BufferProvider& - DiagnosticBufferProvider::build() - { - return diagnostics().reset(); - } - - - DiagnosticBufferProvider& - DiagnosticBufferProvider::access (BufferProvider const& provider) - { - if (!diagnostics().isCurrent (provider)) - throw error::Invalid("given Provider doesn't match (current) diagnostic data record." - "This might be an lifecycle error. Did you build() this instance beforehand?"); - - return diagnostics(); - } - - - - - HeapMemProvider& - DiagnosticBufferProvider::reset() - { - if (pImpl_) - pImpl_->markAllEmitted(); - pImpl_.reset(new HeapMemProvider()); - return *pImpl_; - } - - bool - DiagnosticBufferProvider::isCurrent (BufferProvider const& implInstance) - { - return &implInstance == pImpl_.get(); - } - + DiagnosticBufferProvider::~DiagnosticBufferProvider() + { +// INFO (proc_mem, "discarding %zu diagnostic buffer entries", outSeq_.size()); ////////////OOO implement based on dedicated tracking information in-object + } @@ -86,23 +47,23 @@ namespace engine { /* === diagnostic API === */ bool - DiagnosticBufferProvider::buffer_was_used (uint bufferID) const + BufferDiagnostic::buffer_was_used (uint bufferID) { - return pImpl_->access_emitted(bufferID).was_used(); + return dbp_.heapMemProvider_.access_emitted(bufferID).was_used(); } bool - DiagnosticBufferProvider::buffer_was_closed (uint bufferID) const + BufferDiagnostic::buffer_was_closed (uint bufferID) { - return pImpl_->access_emitted(bufferID).was_closed(); + return dbp_.heapMemProvider_.access_emitted(bufferID).was_closed(); } void* - DiagnosticBufferProvider::accessMemory (uint bufferID) const + BufferDiagnostic::accessMemory (uint bufferID) { - return pImpl_->access_emitted(bufferID).accessMemory(); + return dbp_.heapMemProvider_.access_emitted(bufferID).accessMemory(); } diff --git a/src/steam/engine/diagnostic-buffer-provider.hpp b/src/steam/engine/diagnostic-buffer-provider.hpp index cbf2372ca..54d68fec2 100644 --- a/src/steam/engine/diagnostic-buffer-provider.hpp +++ b/src/steam/engine/diagnostic-buffer-provider.hpp @@ -1,5 +1,5 @@ /* - DIAGNOSTIC-BUFFER-PROVIDER.hpp - helper for testing against the BufferProvider interface + DIAGNOSTIC-BUFFER-PROVIDER.hpp - helper for testing against the BufferProvider interf,ttace Copyright (C) 2011, Hermann Vosseler @@ -22,10 +22,9 @@ #include "lib/error.hpp" -#include "lib/depend.hpp" #include "lib/util.hpp" -#include "steam/engine/type-handler.hpp" -#include "steam/engine/buffer-provider.hpp" +#include "steam/engine/type-handler.hpp" ///////////////OOO warum? +#include "steam/engine/heap-mem-provider.hpp" #include "lib/nocopy.hpp" #include @@ -36,12 +35,8 @@ namespace engine { namespace error = lumiera::error; + class BufferDiagnostic; - /** - * simple BufferProvider implementation - * with additional allocation tracking - */ - class HeapMemProvider; /****************************************************************//** @@ -50,52 +45,49 @@ namespace engine { * @todo write type comment */ class DiagnosticBufferProvider - : util::NonCopyable + : public BufferProvider { - - std::unique_ptr pImpl_; - static lib::Depend diagnostics; + HeapMemProvider heapMemProvider_; - HeapMemProvider& reset(); - bool isCurrent (BufferProvider const&); + /* === delegate BufferProvider API === */ + + uint prepareBuffers (uint count, HashVal typeID) override { return heapMemProvider_.prepareBuffers (count,typeID);} + BuffHandle provideLockedBuffer (HashVal typeID) override { return heapMemProvider_.provideLockedBuffer (typeID); } + void mark_emitted (HashVal h, LocalTag const& t) override { heapMemProvider_.mark_emitted(h,t); } + void detachBuffer (HashVal h, LocalTag const& t, Buff& b) override { heapMemProvider_.detachBuffer(h,t,b); } - DiagnosticBufferProvider(); - ~DiagnosticBufferProvider(); - - friend class lib::DependencyFactory; - public: - /** build a new Diagnostic Buffer Provider instance, - * discard the existing one. Use the static query API - * for investigating collected data. */ - static BufferProvider& build(); - - - /** access the diagnostic API of the buffer provider - * @throw error::Invalid if the given provider doesn't allow - * for diagnostic access or wasn't registered beforehand. - */ - static DiagnosticBufferProvider& - access (BufferProvider const& ); - - - - - /* === diagnostic API === */ - - bool buffer_was_used (uint bufferID) const; - bool buffer_was_closed (uint bufferID) const; - void* accessMemory (uint bufferID) const; - bool all_buffers_released() const; - - + ~DiagnosticBufferProvider(); + DiagnosticBufferProvider(); private: - + /// „backdoor“ to watch instrumentation from tests + friend class BufferDiagnostic; }; + class BufferDiagnostic + : util::MoveOnly + { + DiagnosticBufferProvider& dbp_; + public: + BufferDiagnostic (DiagnosticBufferProvider& thePro) + : dbp_{thePro} + { } + + + bool buffer_was_used (uint bufferID); + bool buffer_was_closed (uint bufferID); + void* accessMemory (uint bufferID); + bool all_buffers_released(); + }; + + inline BufferDiagnostic + watch (BufferProvider& thePro) + { + return BufferDiagnostic{dynamic_cast (thePro)}; + } }} // namespace steam::engine diff --git a/src/steam/engine/heap-mem-provider.cpp b/src/steam/engine/heap-mem-provider.cpp index 995304819..bede22ebd 100644 --- a/src/steam/engine/heap-mem-provider.cpp +++ b/src/steam/engine/heap-mem-provider.cpp @@ -223,15 +223,13 @@ namespace engine { * @internal create a memory tracking BufferProvider, */ HeapMemProvider::HeapMemProvider() - : BufferProvider ("Diagnostic_HeapAllocated") + : BufferProvider ("Naive_HeapAllocated") , pool_(new diagn::PoolTable) , outSeq_() { } - HeapMemProvider::~HeapMemProvider() - { - INFO (proc_mem, "discarding %zu diagnostic buffer entries", outSeq_.size()); - } + + HeapMemProvider::~HeapMemProvider() { /* emit dtor of BlockPool here */ } /* ==== Implementation of the BufferProvider interface ==== */ diff --git a/src/steam/engine/heap-mem-provider.hpp b/src/steam/engine/heap-mem-provider.hpp index a5e212f5d..f5a1011de 100644 --- a/src/steam/engine/heap-mem-provider.hpp +++ b/src/steam/engine/heap-mem-provider.hpp @@ -131,8 +131,8 @@ namespace engine { virtual void detachBuffer (HashVal, LocalTag const&, Buff&) override; public: + ~HeapMemProvider(); HeapMemProvider(); - virtual ~HeapMemProvider(); size_t emittedCnt() const; diff --git a/tests/core/steam/engine/buffer-provider-protocol-test.cpp b/tests/core/steam/engine/buffer-provider-protocol-test.cpp index 73615da0c..396875e8d 100644 --- a/tests/core/steam/engine/buffer-provider-protocol-test.cpp +++ b/tests/core/steam/engine/buffer-provider-protocol-test.cpp @@ -91,7 +91,7 @@ namespace test { // Create Test fixture. // In real usage, a suitable memory/frame/buffer provider // will be preconfigured, depending on the usage context - BufferProvider& provider = DiagnosticBufferProvider::build(); + DiagnosticBufferProvider provider; BuffHandle buff = provider.lockBufferFor(); CHECK (buff.isValid()); @@ -106,11 +106,11 @@ namespace test { CHECK (!buff.isValid()); VERIFY_ERROR (LIFECYCLE, buff.accessAs() ); - DiagnosticBufferProvider& checker = DiagnosticBufferProvider::access(provider); - CHECK (checker.buffer_was_used (0)); - CHECK (checker.buffer_was_closed (0)); + auto diagnostic = watch(provider); + CHECK (diagnostic.buffer_was_used (0)); + CHECK (diagnostic.buffer_was_closed (0)); - CHECK (testData(0) == checker.accessMemory (0)); + CHECK (testData(0) == diagnostic.accessMemory (0)); } @@ -120,7 +120,7 @@ namespace test { verifyRenderingUsage() { // Test fixture: allows to track/verify allocations after the fact - BufferProvider& provider = DiagnosticBufferProvider::build(); + DiagnosticBufferProvider provider; using DataBuff = std::array; constexpr auto BUFFSIZ = sizeof(DataBuff); @@ -174,17 +174,17 @@ namespace test { auto endSum = workBuff.accessAs().getChecksum(); workBuff.release(); // note: not every buffer need be emitted. - DiagnosticBufferProvider& checker = DiagnosticBufferProvider::access(provider); - CHECK (checker.buffer_was_used (0)); - CHECK (checker.buffer_was_closed (0)); -// CHECK (checker.all_buffers_released()); ///////////////OOO do we need this API? + auto diagnostic = watch(provider); + CHECK (diagnostic.buffer_was_used (0)); + CHECK (diagnostic.buffer_was_closed (0)); +// CHECK (diagnostic.all_buffers_released()); ///////////////OOO do we need this API? } void verifyObjectAttachment() { - BufferProvider& provider = DiagnosticBufferProvider::build(); + DiagnosticBufferProvider provider; BuffDescr type_A = provider.getDescriptorFor(sizeof(TestFrame)); BuffDescr type_B = provider.getDescriptorFor(sizeof(int)); BuffDescr type_C = provider.getDescriptor(); @@ -221,7 +221,7 @@ namespace test { void verifyObjectAttachmentFailure() { - BufferProvider& provider = DiagnosticBufferProvider::build(); + DiagnosticBufferProvider provider; BuffDescr type_D = provider.getDescriptorFor(sizeof(Dummy)); Dummy::checksum() = 0; diff --git a/tests/core/steam/engine/node-feed-test.cpp b/tests/core/steam/engine/node-feed-test.cpp index 000c5e6f0..b74ebab0a 100644 --- a/tests/core/steam/engine/node-feed-test.cpp +++ b/tests/core/steam/engine/node-feed-test.cpp @@ -17,6 +17,8 @@ #include "lib/test/run.hpp" +#include "lib/depend-inject.hpp" +#include "steam/engine/engine-ctx.hpp" #include "steam/engine/proc-node.hpp" #include "steam/engine/node-builder.hpp" #include "steam/engine/media-weaving-pattern.hpp" @@ -86,10 +88,10 @@ namespace test { using WeavingPattern = MediaWeavingPattern; using TurnoutWeaving = Turnout; - BufferProvider& provider = DiagnosticBufferProvider::build(); + DiagnosticBufferProvider bufferProvider; Several noLeadPorts; // ◁————————— empty predecessor-port-sequence - Several outBuffDescr = makeSeveral({provider.getDescriptor()}) + Several outBuffDescr = makeSeveral({bufferProvider.getDescriptor()}) .build(); // ◁————————— a single output buffer to hold an `uint` uint resultSlot{0}; @@ -229,7 +231,7 @@ namespace test { .build()}; // Prepare result buffer for invocation - BufferProvider& provider = DiagnosticBufferProvider::build(); ////////////////OOO should rather switch whole pipeline to a DiagnosticBufferProvider here + DiagnosticBufferProvider provider; /////////////////////////OOO should rather switch whole pipeline to a DiagnosticBufferProvider here BuffHandle buff = provider.lockBufferFor (-55); CHECK (-55 == buff.accessAs()); diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 6774bf639..e98b23929 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -111268,8 +111268,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) WER schreibt denn solchen Scheiß Code

- - + @@ -111279,8 +111278,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) schon das Interface BufferProvider hat sehr viel »code behind«

- - + @@ -111403,8 +111401,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) das würde heißen: während der Arbeit an diesen Protokollen habe ich immer in den gleichen Strukturen gedacht, die letztlich Implementierungs-Primitive waren. Dadurch sind Implementierungs-Konzepte in die Standard-Implementierung der Protokolle ge»leaked« — und da es nur diese eine Referenz-Implementierung gab, ist diese Vermischung auch in den »code behind« der Protokoll-Implementierungen eingedrungen. Damit war ich schnell fertig (und stand zudem unter Druck, weil ich nicht vorwärts gekommen bin). Infolgedessen unterblieb eine Kritik, und das herausdestilieren einer abstrakteren Form des Typ-Managers; im Fall der Output-Slots ist es sogar noch schlimmer, denn dort ist eine abstrahierte Implementierung relativ sinnnlos (und würde Vorgriffe machen bezüglich der Technologie, also z.B. dann nur gut passen für Audio-Buffer)

- - + @@ -111416,8 +111413,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) demnach sollte ich aufräumen, nicht neubauen

- - + @@ -111437,16 +111433,66 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - + + - - + + + + + + + + + + +

+ wir haben hier nix zu verbergen +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - + +