From abecd2cbde7a578d32ceeb87493a6c5e1ba1ba31 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Mon, 20 Apr 2026 01:40:15 +0200 Subject: [PATCH] BufferProvider: implement locking and data synchronisation ...using the first usage situation as guideline, and verify proper operation through unit-test - the LocalBufferStage gets a set of thread-local BufferMetadata - furthermore, it accesses the central hub via dependency-injection - after each type registration ''from a worker thread'' data is published As an asside: ensure most metadata registration can ''move'' the TypeHandler --- src/vault/mem/buffer-metadata.hpp | 33 +++-- src/vault/mem/buffer-provider.hpp | 1 + src/vault/mem/diagnostic-buffer-provider.cpp | 8 +- src/vault/mem/engine-buffer-metadata.hpp | 40 +++++- src/vault/mem/local-buffer-stage.hpp | 54 ++++--- .../mem/simple-buffer-state-registry.hpp | 4 +- tests/vault/mem/local-buffer-stage-test.cpp | 50 +++---- wiki/thinkPad.ichthyo.mm | 135 +++++++----------- 8 files changed, 175 insertions(+), 150 deletions(-) diff --git a/src/vault/mem/buffer-metadata.hpp b/src/vault/mem/buffer-metadata.hpp index d6730afa1..87637f56a 100644 --- a/src/vault/mem/buffer-metadata.hpp +++ b/src/vault/mem/buffer-metadata.hpp @@ -84,11 +84,14 @@ #include "lib/util.hpp" #include +#include namespace vault{ namespace mem { + using std::move; + using std::forward; using lib::HashVal; using util::unConst; @@ -157,7 +160,7 @@ namespace mem { namespace { // details of hash calculation template HashVal - chainedHash(HashVal accumulatedHash, VAL changedValue) + chainedHash(HashVal accumulatedHash, VAL const& changedValue) { boost::hash_combine (accumulatedHash, changedValue); return accumulatedHash; @@ -210,7 +213,7 @@ namespace mem { Key (Key const& parent, size_t differingStorageSize) : parent_(parent.hashID_) , hashID_(chainedHash (parent_, differingStorageSize)) - , storageSize_(differingStorageSize) // differing from parent + , storageSize_(differingStorageSize) // ◁────────────────────────────────┨ differing from the parent , instanceFunc_(parent.instanceFunc_) , specifics_(parent.specifics_) { } @@ -220,11 +223,11 @@ namespace mem { * Using different ctor and dtor functions, * all else remaining the same as with parent */ - Key (Key const& parent, TypeHandler const& differingTypeHandlerFunctions) + Key (Key const& parent, TypeHandler differingTypeHandlerFunctions) : parent_(parent.hashID_) , hashID_(chainedHash (parent_, differingTypeHandlerFunctions)) , storageSize_(parent.storageSize_) - , instanceFunc_(differingTypeHandlerFunctions) // differing from parent + , instanceFunc_(move(differingTypeHandlerFunctions)) // ◁─────────────────┨ differing from the parent , specifics_(parent.specifics_) { } @@ -238,7 +241,7 @@ namespace mem { , hashID_(chainedHash (parent_, anotherTypeSpecificInternalTag)) , storageSize_(parent.storageSize_) , instanceFunc_(parent.instanceFunc_) - , specifics_(anotherTypeSpecificInternalTag) // differing from parent + , specifics_(anotherTypeSpecificInternalTag) // ◁───────────────────────┨ differing from the parent { } @@ -600,12 +603,14 @@ namespace mem { * @remark in the Render Engine there can be several * BufferProvider services, while the BufferMetadata * uses a common setup with thread-local caches. + * @note BufferMetadata is *not threadsafe*, thus the workers + * rely on thread-local instances, while the central + * EngineBufferMetadata is protected by a monitor. */ class BufferMetadata : util::NonCopyable { metadata::Table table_; - ///////////////////////////TICKET #854 : ensure proper locking happens "somewhere" when mutating metadata public: using Key = metadata::Key; @@ -629,7 +634,7 @@ namespace mem { * the third level. All these levels describe abstract type * keys, not entries for concrete buffers. The latter are * always created as children of a known type key. - * @param familyID root anchor to keep mandators apart + * @param familyID root anchor to keep the mandators apart */ Key key (HashVal familyID @@ -641,7 +646,7 @@ namespace mem { Key typeKey = trackKey (familyID, storageSize); if (nontrivial(instanceFunc)) - typeKey = trackKey (typeKey, instanceFunc); + typeKey = trackKey (typeKey, move(instanceFunc)); if (nontrivial(specifics)) typeKey = trackKey (typeKey, specifics); @@ -651,9 +656,9 @@ namespace mem { /** create a sub-type, using a different type/handler functor */ Key - key (Key const& parentKey, TypeHandler const& instanceFunc) + key (Key const& parentKey, TypeHandler instanceFunc) { - return trackKey (parentKey, instanceFunc); + return trackKey (parentKey, move(instanceFunc)); } /** create a sub-type, @@ -871,9 +876,9 @@ namespace mem { template Key - trackKey (PAR parent, DEF specialisation) + trackKey (PAR parent, DEF&& specialisation) { - Key newKey{parent, specialisation}; + Key newKey{parent, forward(specialisation)}; maybeStore (newKey); return newKey; } @@ -945,9 +950,9 @@ namespace mem { entry->mark(otherEntry.state()); } else - return true; + return false; // something to synchronise => continue up the path - return false; + return true; } }; diff --git a/src/vault/mem/buffer-provider.hpp b/src/vault/mem/buffer-provider.hpp index 6eb4829d1..136f5de2c 100644 --- a/src/vault/mem/buffer-provider.hpp +++ b/src/vault/mem/buffer-provider.hpp @@ -226,6 +226,7 @@ namespace mem { virtual void discard (HashVal stateKey) =0; virtual bool isAllotted (HashVal stateKey) const =0; virtual bool isAccessible (HashVal stateKey) const =0; + virtual HashVal getFamilyID() const =0; }; class BufferStore diff --git a/src/vault/mem/diagnostic-buffer-provider.cpp b/src/vault/mem/diagnostic-buffer-provider.cpp index bb51f90b2..c0bc8b747 100644 --- a/src/vault/mem/diagnostic-buffer-provider.cpp +++ b/src/vault/mem/diagnostic-buffer-provider.cpp @@ -149,7 +149,6 @@ namespace mem { StageImp stage_; Tracker& tracker_; - DiagnosticBufferProvider& provider_; /* === BufferStage proxy implementation === */ @@ -217,11 +216,12 @@ namespace mem { } public: - InstrumentedStageProxy (StageImp rawImpl, Tracker& tracker, DiagnosticBufferProvider& provider) + InstrumentedStageProxy (StageImp rawImpl, Tracker& tracker) : stage_{move(rawImpl)} , tracker_{tracker} - , provider_{provider} { } + + HashVal getFamilyID() const override { return stage_->getFamilyID(); } }; //(End) internal diagnostics helper @@ -234,7 +234,7 @@ namespace mem { : NaiveBufferSetup{} , tracker_{std::make_unique()} { - decorate (bufferStage_, *tracker_, *this); + decorate (bufferStage_, *tracker_); } DiagnosticBufferProvider::~DiagnosticBufferProvider() diff --git a/src/vault/mem/engine-buffer-metadata.hpp b/src/vault/mem/engine-buffer-metadata.hpp index e61fdd3a5..d03e6d93e 100644 --- a/src/vault/mem/engine-buffer-metadata.hpp +++ b/src/vault/mem/engine-buffer-metadata.hpp @@ -57,6 +57,7 @@ #include "include/logging.h" #include "vault/mem/buffer-metadata.hpp" #include "lib/nocopy.hpp" +#include "lib/sync.hpp" #include "lib/util.hpp" #include @@ -68,18 +69,24 @@ namespace mem { using std::move; using util::unConst; + using lib::Sync; + using lib::RecursiveLock_NoWait; + /** * Exchange hub for buffer metadata in the Render Engine. + * @note all access is threadsafe (protected by mutex) + * @remark in the Render Engine, the bulk of metadata handling + * happens in the worker threads, using LocalBufferStage. */ class EngineBufferMetadata : util::NonCopyable + , public Sync { BufferMetadata registry_; - ///////////////////////////TICKET #854 : ensure proper locking happens "somewhere" when mutating metadata public: using Key = metadata::Key; @@ -99,31 +106,39 @@ namespace mem { bool isKnown (HashVal metaID) const { + Lock sync{this}; return registry_.isKnown (metaID); } bool isAllotted (HashVal metaID) const { + Lock sync{this}; return registry_.isLocked (metaID); } bool isAccessible (HashVal metaID) const { + Lock sync{this}; return registry_.isAccessible (metaID); } Key const& lookup (HashVal metaID) const { + Lock sync{this}; return isKnown (metaID)? registry_.get (metaID) : metadata::Key::INVALID; } Key const& - defineBufferType (HashVal familyID, size_t buffSiz, TypeHandler handlerFunctions, LocalTag localTag) + defineBufferType (HashVal familyID + ,size_t buffSiz + ,TypeHandler handlerFunctions =TypeHandler::RAW + ,LocalTag localTag =LocalTag::UNKNOWN) { + Lock sync{this}; return lookup (registry_.key (familyID, buffSiz, move(handlerFunctions), localTag)); } // possibly create new entry, and return (stable) reference to it @@ -134,6 +149,7 @@ namespace mem { bool abandon (HashVal metaID, bool invokeDtor =true) { + Lock sync{this}; Key const& key = lookup (metaID); Entry& metaEntry = static_cast (unConst(key)); if (metaEntry.isTypeKey()) @@ -142,10 +158,26 @@ namespace mem { registry_.release (metaEntry); return true; } + + + /** push a new metadata chain from the local registry into the central hub. + * @remark typically required after defining a new buffer type locally */ + void + propagateUp (HashVal metaID, BufferMetadata const& localRegistry) + { + Lock sync{this}; + registry_.import(metaID, localRegistry); + } + + /** retrieve a metadata chain from the central registry */ + void + propagateDown (HashVal metaID, BufferMetadata& localRegistry) const + { + Lock sync{this}; + localRegistry.import(metaID, registry_); + } }; - - }} // namespace vault::mem #endif /*VAULT_MEM_ENGINE_BUFFR_METADAZ_H*/ diff --git a/src/vault/mem/local-buffer-stage.hpp b/src/vault/mem/local-buffer-stage.hpp index 854fe63f5..1a37d59d5 100644 --- a/src/vault/mem/local-buffer-stage.hpp +++ b/src/vault/mem/local-buffer-stage.hpp @@ -42,10 +42,11 @@ #define VAULT_MEM_LOCAL_BUFFER_STAGE_H -#include "vault/mem/buffer-metadata.hpp" #include "vault/mem/buffer-provider-setup.hpp" - -#include +#include "vault/mem/engine-buffer-metadata.hpp" +#include "vault/mem/buffer-metadata.hpp" +#include "lib/local-slice.hpp" +#include "lib/depend.hpp" namespace vault { @@ -54,53 +55,67 @@ namespace mem { /** - * Simple Buffer type and state tracking registry, for test and demonstration. - * Relies on a central hashtable, without considering any concurrency concerns. + * Buffer type and state tracking registry, specialised for running + * in a worker thread and within a massively concurrent environment. + * Requests are first and foremost handled by a thread-local registry. + * Metadata is synchronised with a global registry only when types are + * defined anew in the worker, or when a worker refers for the first + * time to an type predefined from the Builder. In those (rare) cases + * however, a mutex synchronisation is necessary to protect the global + * metadata table against corruption and ensure synchronous response: */ class LocalBufferStage : public BufferProviderSetup::Stage { + using LocalRegistry = lib::LocalSlice; + using EngineRegistry = lib::Depend; + HashVal familyID_; - BufferMetadata metadata_; - + LocalRegistry localReg_; + EngineRegistry globalReg_; + + /* === BufferStage interface === */ ID lookup (HashVal key) override { - return metadata_.isKnown(key)? metadata_.get (key) - : metadata::Key::INVALID; + return localReg_->isKnown(key)? localReg_->get (key) + : metadata::Key::INVALID; } bool isAllotted (HashVal stateKey) const override { - return metadata_.isLocked (stateKey); + return localReg_->isLocked (stateKey); } bool isAccessible (HashVal stateKey) const override { - return metadata_.isAccessible (stateKey); + return localReg_->isAccessible (stateKey); } ID defineBufferType (size_t buffSiz, TypeHandler handlerFunctions, LocalTag localTag) { - return lookup (metadata_.key (familyID_, buffSiz, move (handlerFunctions), localTag)); - } // deliberately: maybe create storage, and return reference to it + ID typeKey = lookup (localReg_->key (familyID_, buffSiz, move (handlerFunctions), localTag)); + // possibly create new entry, and retrieve (stable) reference + globalReg_().propagateUp (typeKey, *localReg_); + return typeKey; + } ID mark_locked (ID typeKey, BuffAlloc alloc) override { auto& [storage,size,implMark] = alloc; - return metadata_.markLocked (typeKey, storage, size, implMark); + return localReg_->markLocked (typeKey, storage, size, implMark); } ID mark_emitted (HashVal stateKey) override { - metadata::Entry& metaEntry = metadata_.get (stateKey); + metadata::Entry& metaEntry = localReg_->get (stateKey); metaEntry.mark(EMITTED); return metaEntry; // contains also the key } @@ -108,7 +123,7 @@ namespace mem { ID mark_released (HashVal stateKey) override { - metadata::Entry& metaEntry = metadata_.get (stateKey); + metadata::Entry& metaEntry = localReg_->get (stateKey); metaEntry.mark(FREE); // might invoke embedded dtor function return metaEntry; } @@ -116,7 +131,7 @@ namespace mem { ID abandon (HashVal stateKey, bool invokeDtor) override { - metadata::Entry& metaEntry = metadata_.get (stateKey); + metadata::Entry& metaEntry = localReg_->get (stateKey); metaEntry.invalidate (invokeDtor); return metaEntry; } @@ -124,15 +139,16 @@ namespace mem { void discard (HashVal stateKey) override { - metadata_.release (stateKey); + localReg_->release (stateKey); } public: LocalBufferStage (Literal implementationID) : familyID_{hash_value (implementationID)} - , metadata_{} { } + + HashVal getFamilyID() const override { return familyID_; } }; diff --git a/src/vault/mem/simple-buffer-state-registry.hpp b/src/vault/mem/simple-buffer-state-registry.hpp index 8819ceee6..6ca9a3933 100644 --- a/src/vault/mem/simple-buffer-state-registry.hpp +++ b/src/vault/mem/simple-buffer-state-registry.hpp @@ -27,8 +27,6 @@ #include "vault/mem/buffer-metadata.hpp" #include "vault/mem/buffer-provider-setup.hpp" -#include - namespace vault { namespace mem { @@ -115,6 +113,8 @@ namespace mem { : familyID_{hash_value (implementationID)} , metadata_{} { } + + HashVal getFamilyID() const override { return familyID_; } }; diff --git a/tests/vault/mem/local-buffer-stage-test.cpp b/tests/vault/mem/local-buffer-stage-test.cpp index 8c58d3e06..1ead7d55f 100644 --- a/tests/vault/mem/local-buffer-stage-test.cpp +++ b/tests/vault/mem/local-buffer-stage-test.cpp @@ -56,15 +56,6 @@ namespace test { namespace { // Test fixture - template - TY& - accessAs (metadata::Entry& entry) - { - TY* ptr = reinterpret_cast (entry.access()); - ASSERT (ptr); - return *ptr; - } - template Buff* mark_as_Buffer(X& something) @@ -72,16 +63,6 @@ namespace test { return reinterpret_cast (std::addressof(something)); } -// struct TestBuff -// : lib::UninitialisedStorage -// { -// -// }; - const size_t TEST_MAX_SIZE = 1024 * 1024; - - HashVal JUST_SOMETHING = 123; - auto SOME_POINTER = mark_as_Buffer(JUST_SOMETHING); - }//(End) Test fixture and helpers @@ -108,6 +89,7 @@ namespace test { } + /** @test key-chains can be exported and imported */ void verify_DataTransfer() @@ -182,6 +164,7 @@ namespace test { } + /** @test simulate a common buffer provider usage situation, * whereby a local instance in some worker thread provides * a new (or refined) buffer type. @@ -191,16 +174,17 @@ namespace test { * - notably register a totally new buffer type, from within the worker * - then also perform the steps necessary to lock and release a buffer * - check that the new type was migrated into the central metadata hub + * - verify that also the complete paretn-chain was published */ void verify_publishNewKey() { - // provide a »central Metadata hub« + // provide a »central Metadata hub« (as a transient instance for this test) lib::DependInject::Local metaHub; metaHub.triggerCreate(); - CHECK (0 == metaHub->cntEntries()); + CHECK (0 == metaHub->cntEntries()); // no buffer metadata yet... - // provide the test subject, + // provide the *test subject*, // which implements the BufferStage API // and uses a thread-local metadata table internally LocalBufferStage localStage{"verify_publishNewKey"}; @@ -218,9 +202,9 @@ namespace test { double testBuffer{0}; Buff* alloc = mark_as_Buffer (testBuffer); - Thread testWorker{[&] + Thread testWorker{[&] /* === Use Case : a worker defines a new buffer type === */ { - auto& typeKey = stageAPI.defineBufferType (SIZ_D, HANDLER, TAG); + auto& typeKey = stageAPI.defineBufferType (SIZ_D, HANDLER, TAG); // ◁─────────────────┨ data up-sync happens here metaID = HashVal(typeKey); // sneak out to verify later // emulate allocation and usage of a buffer @@ -232,15 +216,27 @@ namespace test { // release the buffer... stageAPI.mark_released (stateKey); - stageAPI.discard (stateKey); + stageAPI.discard (stateKey); // ◁──────────────────────────────────────────────────┨ buffer state entry never leaves the worker thread }}; while (testWorker) yield(); // wait for worker to finish CHECK (0 != metaID); -SHOW_EXPR(metaHub->cntEntries()); - /////////////////////////////////////////OOO verify the complete chain from the type key to the root + CHECK (3 == metaHub->cntEntries()); + CHECK (metaHub->isKnown (metaID)); + + // complete registration chain has been published to the central registry + HashVal FAM_ID = localStage.getFamilyID(); + HashVal parent1 = metaHub->lookup(metaID).parentKey(); + CHECK (metaHub->isKnown (parent1)); + CHECK (parent1 == metaHub->defineBufferType (FAM_ID, SIZ_D, HANDLER)); + + HashVal parent0 = metaHub->lookup(parent1).parentKey(); + CHECK (metaHub->isKnown (parent0)); + CHECK (parent0 == metaHub->defineBufferType (FAM_ID, SIZ_D)); + + CHECK (FAM_ID == metaHub->lookup(parent0).parentKey()); } diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index de0e58e91..23b515d25 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -144225,9 +144225,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

Das erfordert Vorsicht, weil man dadurch die Eigenschaft einer geschlossenen Service-Instanz verliert; es ist also leider kein drop-in-replacement, mit dem man einfach eine tief eingebettete Koponente thread-local macht; theoretisch würde letzteres zwar gehen, aber dann auf einen zweistufigen Dispatch hinauslaufen, d.h. nach dem thread-local-Zugriff müßte noch ein Dispatch über eine Map per umschließender Service-Instanz erfolgen. @@ -144338,9 +144336,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

man muß den aktuellen Namespace schließen, in lib gehen, dort die explizite Spezialisierung definieren... @@ -144354,9 +144350,7 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

direkt bei LocalSlice definiert; erzeugt eine default-konstruierte Instanz vom Service @@ -144425,46 +144419,38 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

Das bedeutet, die hier aufgestellten Regeln/Verbote sind so konstruiert, daß sie bei einem sinnvollen Abgleich niemals auftreten können.  Der Sinn ergibt sich dabei aus den vorgesehenen Zustandsübergängen; es sollten nur Typ-Informationen angereichert werden, und konkrete Einträge sollten eigentlich gar nicht synchronisiert werden; hilfsweise sehe ich die Möglichkeit vor, daß konkrete Einträge sich gemäß State-Machine vorwärts bewegen (falls wir tatsächlich konkrete Einträge zentral ablegen, wenn sie in den Cache wandern — ich kann diese Möglichkeit derzeit nicht ausschließen)

- - + - + + - - - +

es sollte nur alloziert werden, wenn das auch notwendig ist; man könnte entweder einen Buffer vorsehen, oder direkt von Quelle zu Ziel arbeiten.

- - + - - - +

es müssen keine Komponenten gegeneinander isoliert werden; die Schnittstelle dient nur dazu, den vorgang leicht beherrschbar zu machen

- - + @@ -144477,16 +144463,13 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

...damit kann man in der Regel das rekursive Traversieren der Kette vorzeitig abbrechen; das beruht auf einem »Invarianten-Argument«: Daten in der Registry sind stets konsistent, was impliziert, daß wir auch den Parent-Key bereits gespeichert haben, entweder durch reguläres Anlegen, oder durch eine vorausgegangenen Synchronisation

- - + @@ -144513,35 +144496,36 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

also entweder wenn jeman eine lokale Instanz aufruft, um einen neuen Typ zu registrieren, oder wenn ein lokal nicht bekannter type-Key auftritt

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

soll auf jeden Fall ausschließen, daß dieser Test die zentral-Instanz verschmutzt

- - + @@ -144555,28 +144539,27 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - + - - + + - - + + + + - - - +

Ich verwende hier einen TypeHandler, der einen vorher im Test festgelegten random-double-Wert in den buffer pflanzt. Da wir bisher noch den Bug mit dem TypeHandler-Aliasing haben, gehat das »knapp am Abgrund vorbei« (aber wir verwenden ja eine komplett leere BufferMetadata registry....

- - + @@ -144649,36 +144632,29 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) - - - +

zwar könnte man einfach eine Instanz von BufferMetadata hinstellen — ich halte das aber nicht für klug, denn BufferMetadata ist Implementierungs-Code (und ich bin nicht glücklich damit, wiewohl ich keine Argumente dagegen finde)

- - + - + - - - +

Keine Funktionalität zum Bedienen konkreter Allokationen

- - + + - - - +
  • @@ -144695,35 +144671,34 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) + + + + - - - +

    darf also keinerlei Funktionalität aus dem Steam-Layer voraussetzen

    - -     +     - - - +

    das hier ist keine generische Library, sondern ein Application-Framework. Auch »Vault« ist inhaltlich genau auf Lumiera ausgerichtet, der Unterschied liegt nur im Grad der Integration mit Application-level Services

    - -     +     - - + + +