From 76542e873afd5c50538231bed6639ea247345789 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Tue, 12 May 2026 23:52:25 +0200 Subject: [PATCH] BufferProvider: decide how to introduce locking I prefer to favour a small number of locks at API level. See https://issues.lumiera.org/ticket/1429#comment:1 --- src/vault/mem/engine-buffer-manager.cpp | 11 ++- src/vault/mem/engine-buffer-manager.hpp | 27 +++--- .../vault/mem/engine-buffer-manager-test.cpp | 38 +++++++-- wiki/thinkPad.ichthyo.mm | 85 +++++++++++++++++-- 4 files changed, 135 insertions(+), 26 deletions(-) diff --git a/src/vault/mem/engine-buffer-manager.cpp b/src/vault/mem/engine-buffer-manager.cpp index 3275bdc06..c2d576754 100644 --- a/src/vault/mem/engine-buffer-manager.cpp +++ b/src/vault/mem/engine-buffer-manager.cpp @@ -71,9 +71,17 @@ namespace mem { /* === EngineBufferManager implementation === */ + /** + * @note 5/2026 I prefer a small number of locks at API level, + * based on the assumption that this processing environment + * does not exhibit much contention. This should be [validated] however. + * [validated]: https://issues.lumiera.org/ticket/1429#comment:1 + */ void - EngineBufferManager::handleAllocRequests() + EngineBufferManager::processPendingRequests() { + Lock sync{this}; + inQueue_ .consume_all ([this](Alloc alloc){ doRedeemAllocation (alloc); }); requestQueue_.consume_all ([this](AllocRequest request) { REQUIRE (request.receiver); @@ -87,6 +95,7 @@ namespace mem { Alloc EngineBufferManager::requestAllocation (size_t sizRequest) { + Lock sync{this}; processPendingRequests(); return doPerformAllocation (sizRequest); } diff --git a/src/vault/mem/engine-buffer-manager.hpp b/src/vault/mem/engine-buffer-manager.hpp index c4baba0ea..6f27e2531 100644 --- a/src/vault/mem/engine-buffer-manager.hpp +++ b/src/vault/mem/engine-buffer-manager.hpp @@ -45,6 +45,7 @@ //#include "vault/mem/type-handler.hpp" //#include "vault/mem/buffer-local-tag.hpp" #include "lib/nocopy.hpp" +#include "lib/sync.hpp" //#include "lib/util.hpp" #include @@ -66,6 +67,9 @@ namespace mem { } + using lib::Sync; + using lib::RecursiveLock_NoWait; + struct Alloc { Buff* mem; @@ -110,6 +114,7 @@ namespace mem { */ class EngineBufferManager : public AllocReceiver + , public Sync { struct AllocRequest { @@ -133,7 +138,7 @@ namespace mem { size_t size() const; - bool empty() const; + bool empty() const; /****************************************************************//** @@ -166,9 +171,6 @@ namespace mem { void processPendingRequests(); private: - /** process all pending requests in the queue */ - void handleAllocRequests(); - /** performs the actual allocation work */ Alloc doPerformAllocation (size_t sizRequest); void doRedeemAllocation (Alloc); @@ -184,13 +186,6 @@ namespace mem { } - inline void - EngineBufferManager::processPendingRequests() - { - inQueue_.consume_all ([this](Alloc alloc){ doRedeemAllocation (alloc); }); - handleAllocRequests(); - } - /** wrapper to inspect internals from a unit test */ @@ -219,13 +214,19 @@ namespace mem { size_t bytesAllocd() { - UNIMPLEMENTED ("alloc state diagnostic"); + return globalPool_.allocated_; } size_t bytesLeased() { - UNIMPLEMENTED ("alloc state diagnostic"); + return globalPool_.leased_; + } + + size_t + numAllocs() + { + return globalPool_.cnt_; } /////////////////////////////////////////////////////////////////////////////////////////////////////TICKET #1430 : add pool diagnostics here if we actually switch to pooled allocations diff --git a/tests/vault/mem/engine-buffer-manager-test.cpp b/tests/vault/mem/engine-buffer-manager-test.cpp index ef3527bed..f06d495e2 100644 --- a/tests/vault/mem/engine-buffer-manager-test.cpp +++ b/tests/vault/mem/engine-buffer-manager-test.cpp @@ -111,27 +111,55 @@ namespace test { - /** @test show that a direct request for allocation is honored */ + /** @test show that a direct request for allocation is honored + * - request some memory + * - read and write to that memory + * - return all leased allocations + * - verify the diagnostic meshes up with the allocation + * @remark in principle it is not possible to prove that some + * memory allocation actually works, other than checking + * that access to the given memory is possible, because + * the fact that no memory corruption took place is + * only semi-decidable (you can show a corruption event, + * but not the absence of any such an event). + */ void verify_syncRequest() { EngineBufferManager manager; -SHOW_EXPR(watch(manager).isEmpty()) + CHECK (watch(manager).isEmpty()); Alloc alloc = manager.requestAllocation (555); CHECK (not alloc.empty()); CHECK (alloc.siz >= 555); -SHOW_EXPR(watch(manager).bytesAllocd()) -SHOW_EXPR(watch(manager).bytesLeased()) + CHECK (alloc.siz == watch(manager).bytesAllocd()); + CHECK (alloc.siz == watch(manager).bytesLeased()); + CHECK (alloc.siz % HeapBufferAllocator::TILE_SIZ == 0); // can use that memory char* buf = reinterpret_cast (alloc.mem); string ranS{lib::randStr(555-1)}; auto* p = std::copy (ranS.begin(), ranS.end(), buf); -SHOW_EXPR(p - buf) + CHECK (ranS.length() == size_t(p - buf)); *p = '\0'; CHECK (ranS == buf); + + CHECK (1 == watch(manager).numAllocs()); + Alloc a2 = manager.requestAllocation (1'000'000); + CHECK (not a2.empty()); + CHECK (a2.siz >= 1e6); + CHECK (2 == watch(manager).numAllocs()); + CHECK (1e6+555 < watch(manager).bytesLeased()); + + manager.supply (a2); + manager.processPendingRequests(); + CHECK (1 == watch(manager).numAllocs()); + CHECK (1e6 > watch(manager).bytesLeased()); + manager.supply (alloc); + manager.processPendingRequests(); + CHECK (0 == watch(manager).numAllocs()); + CHECK (0 == watch(manager).bytesLeased()); } diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 1f64aab39..5c981bc4b 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -146736,7 +146736,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - + @@ -146761,7 +146761,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - + @@ -146771,6 +146771,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) + @@ -146814,6 +146815,58 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) + + + + + + + + +

+ im Grunde müßte man nur den Zugang zum Memory-Manager selber locken, denn die ganzen Queue-Operationen sind ja schon lock-free und threadsafe; auch könnten diese Locks dann non-recursive sein +

+ + + + + + + + + + +

+ ...andererseits ist jeder (Mutex)-Lock ein Kernel-Aufruf, und wir erwarten durchaus ganze Bündel an Requests. Außerdem wird mit wenigen Locks auf API-Level viel klarer, daß das Locking vollständig, und der Zustand konsistent ist. +

+ + + + + + + + + + + +

+ ...und zwar sagt mir mein Bauchgefühl, daß wir hier keine Situation mit starker Contention haben; das hat sich auch bereits im Zusammenhang mit dem Scheduler gezeigt. Im Regelfall erwarte ich, daß die Anfragen durch die Queue gehen, und damit ohne Interferenz aneinander vorbeikommen. Wenn dann tatsächlich mal ein Thread den synchronen Pfad einschlägt, erscheint es mir sinnvoller, alle Arbeit in einem Schwung zu erledigen, selbst wenn dadruch (ohenhin nur ausnahmsweise!) ein anderer Thread mal geblockt wird. Letztlich gilt hier immer: die Jobs sind vergleichsweise groß. +

+

+ +

+

+ Den Ausschlag gegeben hat aber eine andere Hinsicht: Klarheit und Wartbarkeit sind wichtiger, spezielle Performance-Tweaks sollte man empirisch beweisen. +

+ + + + + + + + @@ -146831,11 +146884,10 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - + + - - + @@ -146858,6 +146910,9 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) + + + @@ -171564,7 +171619,7 @@ std::cout << tmpl.render({"what", "World"}) << s - + @@ -171583,6 +171638,22 @@ std::cout << tmpl.render({"what", "World"}) << s + + + + + + +

+ ...nämlich indem der Job letztlich auf den synchronen Fall zurückgreift, dieser aber durch die gleiche Mechanik implementiert ist, wie der asynchrone Fall, und dabei gleich alle aufgesammelten Requests in den Queues mit verarbeitet. Dadruch wird effektiv ein Batch an Allokator-Aufwand in denjenigen Worker geschoben, der rein zufällig als erster auf eine nicht bediente Allokation stößt. Das Schöne an dieser Code-Struktur ist, daß die Möglichkeit offen bleibt, später zur Performance-Optimierung (oder nur in bestimmten Situationen) dennoch etwas speziell einzurichten / schedulen / auszuführen, ohne daß dazu der Code im EngineBufferManager geändert werden muß +

+ + + + + + +