From 39e9ecd90e6fe60fadf767ef8b885acdddfee047 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Wed, 19 Jun 2024 15:22:26 +0200 Subject: [PATCH] Library: `AllocationCluster` and `SeveralBuilder` logic tweaks ...identified as part of bug investigation * make clear that reserve() prepares for an absolute capacity * clarify that, to the contrary, ensureStorageCapaciy() means the delta Moreover, it turns out that the assertion regarding storage limits triggers frequently while writing the test code; so we can conclude that the `AllocationCluster` interface lures into allocating without previous check. Consequently, this check now throws a runtime exception. As an aside, the size limitation should be accessible on the interface, similar to `std::vector::max_size()` --- src/lib/allocation-cluster.cpp | 62 +++++++----- src/lib/allocation-cluster.hpp | 24 ++++- src/lib/several-builder.hpp | 8 +- src/lib/util-quant.hpp | 7 ++ src/lib/util.hpp | 8 ++ tests/library/allocation-cluster-test.cpp | 18 ++-- tests/library/several-builder-test.cpp | 13 ++- wiki/thinkPad.ichthyo.mm | 117 +++++++++++++++++++++- 8 files changed, 217 insertions(+), 40 deletions(-) diff --git a/src/lib/allocation-cluster.cpp b/src/lib/allocation-cluster.cpp index fbac18787..929231772 100644 --- a/src/lib/allocation-cluster.cpp +++ b/src/lib/allocation-cluster.cpp @@ -40,20 +40,20 @@ #include "lib/allocation-cluster.hpp" #include "lib/linked-elements.hpp" +#include "lib/format-string.hpp" +#include "lib/util-quant.hpp" #include "lib/util.hpp" using util::unConst; +using util::isPow2; using util::isnil; +using util::_Fmt; using std::byte; namespace lib { - namespace {// Configuration constants - const size_t EXTENT_SIZ = 256; - const size_t OVERHEAD = 2 * sizeof(void*); - const size_t STORAGE_SIZ = EXTENT_SIZ - OVERHEAD; - + namespace {// Internals... /** * Special allocator-policy for lib::LinkedElements @@ -77,7 +77,10 @@ namespace lib { } }; - } + }//(End)configuration and internals + + + /** @@ -105,7 +108,7 @@ namespace lib { { Extent* next; Destructors dtors; - std::byte storage[STORAGE_SIZ]; + std::byte storage[max_size()]; }; using Extents = lib::LinkedElements; @@ -167,8 +170,8 @@ namespace lib { size_t calcAllocInCurrentBlock() const { - ENSURE (STORAGE_SIZ >= view_.storage.rest); - return STORAGE_SIZ - view_.storage.rest; + ENSURE (max_size() >= view_.storage.rest); + return max_size() - view_.storage.rest; } @@ -197,7 +200,7 @@ namespace lib { { view_.extents.emplace(); view_.storage.pos = & view_.extents.top().storage; - view_.storage.rest = STORAGE_SIZ; + view_.storage.rest = max_size(); } }; @@ -246,7 +249,7 @@ namespace lib { void AllocationCluster::expandStorage (size_t allocRequest) { - ENSURE (allocRequest + OVERHEAD <= EXTENT_SIZ); + ENSURE (allocRequest <= max_size()); StorageManager::access(*this).addBlock(); } @@ -259,21 +262,34 @@ namespace lib { - - /* === diagnostics helpers === */ - - bool - AllocationCluster::_is_within_limits (size_t size, size_t align) + /** + * Allocation cluster uses a comparatively small tile size for its extents, + * which turns out to be a frequently encountered limitation in practice. + * This was deemed acceptable, due to its orientation towards performance. + * @throws err::Fatal when a desired allocation can not be accommodated + */ + void + AllocationCluster::__enforce_limits (size_t allocSiz, size_t align) { - auto isPower2 = [](size_t n){ return !(n & (n-1)); }; - return 0 < size - and 0 < align - and size <= STORAGE_SIZ - and align <= STORAGE_SIZ - and isPower2 (align); + REQUIRE (allocSiz); + REQUIRE (align); + REQUIRE (isPow2 (align)); + + if (allocSiz > max_size()) + throw err::Fatal{_Fmt{"AllocationCluster: desired allocation of %d bytes " + "exceeds the fixed extent size of %d"} % allocSiz % max_size() + ,LERR_(CAPACITY)}; + + if (align > max_size()) + throw err::Fatal{_Fmt{"AllocationCluster: data requires alignment at %d bytes, " + "which is beyond the fixed extent size of %d"} % align % max_size() + ,LERR_(CAPACITY)}; } + + /* === diagnostics helpers === */ + size_t AllocationCluster::numExtents() const { @@ -291,7 +307,7 @@ namespace lib { size_t extents = numExtents(); if (not extents) return 0; size_t bytes = StorageManager::access (unConst(*this)).calcAllocInCurrentBlock(); - return (extents - 1) * STORAGE_SIZ + bytes; + return (extents - 1) * max_size() + bytes; } diff --git a/src/lib/allocation-cluster.hpp b/src/lib/allocation-cluster.hpp index 43e0ab920..3bf606879 100644 --- a/src/lib/allocation-cluster.hpp +++ b/src/lib/allocation-cluster.hpp @@ -115,6 +115,11 @@ namespace lib { AllocationCluster (); ~AllocationCluster () noexcept; + /** hard wired size of storage extents */ + static size_t constexpr EXTENT_SIZ = 256; + static size_t constexpr max_size(); + + /* === diagnostics === */ size_t numExtents() const; size_t numBytes() const; @@ -161,7 +166,7 @@ namespace lib { void* allotMemory (size_t bytes, size_t alignment) { - ENSURE (_is_within_limits (bytes, alignment)); + __enforce_limits (bytes, alignment); void* loc = storage_.allot(bytes, alignment); if (loc) return loc; expandStorage (bytes); @@ -201,7 +206,7 @@ namespace lib { void expandStorage (size_t); void registerDestructor (Destructor&); - bool _is_within_limits (size_t,size_t); + void __enforce_limits (size_t,size_t); friend class test::AllocationCluster_test; }; @@ -212,6 +217,21 @@ namespace lib { //-----implementation-details------------------------ + /** + * Maximum individual allocation size that can be handled. + * @remark AllocationCluser expands its storage buffer in steps + * of fixed sized _tiles_ or _extents._ Doing so can be beneficial + * when clusters are frequently created and thrown away (which is the + * intended usage pattern). However, using such extents is inherently + * wasteful, and thus the size must be rather tightly limited. + */ + size_t constexpr + AllocationCluster::max_size() + { + size_t ADMIN_OVERHEAD = 2 * sizeof(void*); + return EXTENT_SIZ - ADMIN_OVERHEAD; + } + /** * Factory function: place a new instance into this AllocationCluster, * but *without invoking its destructor* on clean-up (for performance reasons). diff --git a/src/lib/several-builder.hpp b/src/lib/several-builder.hpp index cf818fd40..44adf2c92 100644 --- a/src/lib/several-builder.hpp +++ b/src/lib/several-builder.hpp @@ -136,6 +136,7 @@ namespace lib { using util::max; using util::min; using util::_Fmt; + using util::positiveDiff; using std::is_nothrow_move_constructible_v; using std::is_trivially_move_constructible_v; using std::is_trivially_destructible_v; @@ -369,14 +370,15 @@ namespace lib { auto withAllocator (ARGS&& ...args); - /** ensure sufficient memory allocation up-front */ + /** ensure up-front that a desired capacity is allocated */ template SeveralBuilder&& reserve (size_t cntElm =1 ,size_t elmSiz =reqSiz()) { + size_t extraElm = positiveDiff (cntElm, Coll::size()); ensureElementCapacity (elmSiz); - ensureStorageCapacity (elmSiz,cntElm); + ensureStorageCapacity (elmSiz,extraElm); elmSiz = max (elmSiz, Coll::spread()); adjustStorage (cntElm, elmSiz); return move(*this); @@ -503,7 +505,7 @@ namespace lib { % util::typeStr() % requiredSiz % Coll::spread()}; } - /** ensure sufficient storage reserve or verify the ability to re-allocate */ + /** ensure sufficient storage reserve for \a newElms or verify the ability to re-allocate */ template void ensureStorageCapacity (size_t requiredSiz =reqSiz(), size_t newElms =1) diff --git a/src/lib/util-quant.hpp b/src/lib/util-quant.hpp index c1bded518..4dd9751a5 100644 --- a/src/lib/util-quant.hpp +++ b/src/lib/util-quant.hpp @@ -38,6 +38,13 @@ namespace util { + template + inline bool constexpr + isPow2 (N n) + { + return n > 0 and !(n & (n-1)); + }; // at each power of 2, a new bit is set for the first time + /** helper to treat int or long division uniformly */ template diff --git a/src/lib/util.hpp b/src/lib/util.hpp index dffb47a4e..1282e8b1d 100644 --- a/src/lib/util.hpp +++ b/src/lib/util.hpp @@ -112,6 +112,14 @@ namespace util { and val <= upperBound; } + template + inline UN constexpr + positiveDiff (N2 newVal, UN refVal) + { + return UN(newVal) > refVal? UN(newVal) - refVal + : UN(0); + } + /** positive integral number from textual representation * @return always a number, 0 in case of unparseable text, * limited to 0 <= num <= LUMIERA_MAX_ORDINAL_NUMBER */ diff --git a/tests/library/allocation-cluster-test.cpp b/tests/library/allocation-cluster-test.cpp index 325563314..9bca60770 100644 --- a/tests/library/allocation-cluster-test.cpp +++ b/tests/library/allocation-cluster-test.cpp @@ -63,7 +63,7 @@ namespace test { const uint NUM_TYPES = 20; const uint NUM_OBJECTS = 500; - const size_t BLOCKSIZ = 256; ///< @warning actually defined in allocation-cluster.cpp + const size_t EXTSIZ = AllocationCluster::EXTENT_SIZ; int64_t checksum = 0; // validate proper pairing of ctor/dtor calls @@ -242,17 +242,17 @@ namespace test { CHECK (2 == clu.numBytes()); CHECK (clu.storage_.pos != nullptr); CHECK (clu.storage_.pos == (& i1) + 1 ); // points directly behind the allocated integer - CHECK (clu.storage_.rest == BLOCKSIZ - (2*sizeof(void*) + sizeof(uint16_t))); + CHECK (clu.storage_.rest == EXTSIZ - (2*sizeof(void*) + sizeof(uint16_t))); // Demonstration: how to reconstruct the start of the current extent byte* blk = static_cast(clu.storage_.pos); - blk += clu.storage_.rest - BLOCKSIZ; + blk += clu.storage_.rest - EXTSIZ; CHECK(size_t(blk) < size_t(clu.storage_.pos)); // some abbreviations for navigating the raw storage blocks... auto currBlock = [&]{ byte* blk = static_cast(clu.storage_.pos); - blk += clu.storage_.rest - BLOCKSIZ; + blk += clu.storage_.rest - EXTSIZ; return blk; }; auto posOffset = [&]{ @@ -273,7 +273,7 @@ namespace test { uint16_t i1pre = i1; auto& i2 = clu.create (55555); CHECK (posOffset() == 2 * sizeof(void*) + 2 * sizeof(uint16_t)); - CHECK (clu.storage_.rest == BLOCKSIZ - posOffset()); + CHECK (clu.storage_.rest == EXTSIZ - posOffset()); // existing storage unaffected CHECK (i1 == i1pre); CHECK (i2 == 55555); @@ -295,8 +295,8 @@ namespace test { for (uint i=clu.storage_.rest; i>0; --i) clu.create (i); CHECK (clu.storage_.rest == 0); // no space left in current extent - CHECK (posOffset() == BLOCKSIZ); - CHECK (clu.numBytes() == BLOCKSIZ - 2*sizeof(void*)); // now using all the rest behind the admin »slots« + CHECK (posOffset() == EXTSIZ); + CHECK (clu.numBytes() == EXTSIZ - 2*sizeof(void*)); // now using all the rest behind the admin »slots« CHECK (clu.numExtents() == 1); CHECK (slot(0) == 0); CHECK (blk == currBlock()); // but still in the initial extent @@ -305,8 +305,8 @@ namespace test { char& c2 = clu.create ('U'); CHECK (blk != currBlock()); // allocation moved to a new extent CHECK (getAddr(c2) == currBlock() + 2*sizeof(void*)); // c2 resides immediately after the two administrative »slots« - CHECK (clu.storage_.rest == BLOCKSIZ - posOffset()); - CHECK (clu.numBytes() == BLOCKSIZ - 2*sizeof(void*) + 1); // accounted allocation for the full first block + one byte + CHECK (clu.storage_.rest == EXTSIZ - posOffset()); + CHECK (clu.numBytes() == EXTSIZ - 2*sizeof(void*) + 1); // accounted allocation for the full first block + one byte CHECK (clu.numExtents() == 2); // we have two extents now CHECK (slot(0) == size_t(blk)); // first »slot« of the current block points back to previous block CHECK (i1 == i1pre); diff --git a/tests/library/several-builder-test.cpp b/tests/library/several-builder-test.cpp index 4d5c818f3..173e42c21 100644 --- a/tests/library/several-builder-test.cpp +++ b/tests/library/several-builder-test.cpp @@ -610,8 +610,8 @@ namespace test{ { auto builder = makeSeveral() .withAllocator(clu) - .reserve(5) - .fillElm(5); + .reserve(4) + .fillElm(4); size_t buffSiz = sizeof(Dummy) * builder.capacity(); size_t headerSiz = sizeof(ArrayBucket); @@ -631,6 +631,15 @@ SHOW_EXPR(buffSiz + headerSiz) SHOW_EXPR(builder.size()) SHOW_EXPR(builder.capacity()) SHOW_EXPR(clu.numExtents()) +SHOW_EXPR(clu.numBytes()) + // now perform another unrelated allocation + Dummy& extraDummy = clu.create(55); +SHOW_EXPR(clu.numExtents()) +SHOW_EXPR(clu.numBytes()) + builder.reserve(9); +SHOW_EXPR(builder.size()) +SHOW_EXPR(builder.capacity()) +SHOW_EXPR(clu.numExtents()) SHOW_EXPR(clu.numBytes()) } } diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index df11558d6..8b9ea2bd3 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -84196,8 +84196,123 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
+ + - + + + + + + + + +

+ denn das eine zusätzliche Element würde locker reinpassen — und zudem sollte ja nun der Block in den nächsten Extent migrieren +

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

+ einmal aus emplaceNewElm() und einmal aus reserve() +

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

+ das ist es nämlich, worauf die Implementierung hinausläuft, und dieser Grebrauch ist auch naheliegend und korrekt, sofern es sich um das Einfügen neuer Datenelemente handelt. Insofern muß sich der andere Gebrauch in reserve() daran anpassen +

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

+ ...daß da immer noch high-level-Code darüber liegt, der eine inhaltliche Prüfung gemacht hat, so daß hier nur noch ein Konsistenzcheck vonnöten ist +

+ + + + + + + + + + +

+ ...aber realistisch betrachtet ist der AllocationCluster viel zu einfach zu verwenden, als daß man da überhaupt daran denkt, noch explizit zu prüfen — und durch den standard-Allocator-Adapter gibt es unvermeidbar einen direkten Eingang über allot(). +

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