From d9f9cc40836f282a5ebf914980485245f342f149 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Thu, 16 Dec 2021 15:25:26 -0500 Subject: [PATCH] ObjectPool: Use Platform::New and rename modes (#13088) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Problem - Heap allocations should use `Platform::New` rather than raw `new`. - `ObjectPoolMem::kStatic` could be misleading, since the pool memory is part of the enclosing context, not static unless also declared `static`. #### Change overview - Use `Platform::New` and `Platform::Delete`. - Rename `ObjectPoolMem::kStatic` → `kInline` and `kDynamic` → `kHeap`. #### Testing CI --- src/lib/dnssd/minimal_mdns/Server.h | 2 +- .../minimal_mdns/tests/CheckOnlyServer.h | 2 +- src/lib/support/Pool.cpp | 2 +- src/lib/support/Pool.h | 30 +++++++++++++------ src/lib/support/tests/TestPool.cpp | 27 +++++++++-------- src/system/tests/TestSystemTimer.cpp | 6 ++++ src/transport/raw/TCP.h | 2 +- 7 files changed, 45 insertions(+), 26 deletions(-) diff --git a/src/lib/dnssd/minimal_mdns/Server.h b/src/lib/dnssd/minimal_mdns/Server.h index 9c77dbec5fdd94..46c1ae8c5c606c 100644 --- a/src/lib/dnssd/minimal_mdns/Server.h +++ b/src/lib/dnssd/minimal_mdns/Server.h @@ -228,7 +228,7 @@ class ServerBase // The PoolImpl impl is used as a base class because its destructor must be called after ServerBase's destructor. template -class Server : private chip::PoolImpl, public ServerBase { diff --git a/src/lib/dnssd/minimal_mdns/tests/CheckOnlyServer.h b/src/lib/dnssd/minimal_mdns/tests/CheckOnlyServer.h index 94e934cc22d185..3e468f3e6238a3 100644 --- a/src/lib/dnssd/minimal_mdns/tests/CheckOnlyServer.h +++ b/src/lib/dnssd/minimal_mdns/tests/CheckOnlyServer.h @@ -71,7 +71,7 @@ void MakePrintableName(char (&location)[N], FullQName name) } // namespace -class CheckOnlyServer : private chip::PoolImpl, public ServerBase, public ParserDelegate, diff --git a/src/lib/support/Pool.cpp b/src/lib/support/Pool.cpp index e1e64f3d3f8170..4f44edaf97d1c7 100644 --- a/src/lib/support/Pool.cpp +++ b/src/lib/support/Pool.cpp @@ -151,7 +151,7 @@ Loop HeapObjectList::ForEachNode(void * context, Lambda lambda) if (p->mObject == nullptr) { p->Remove(); - delete p; + Platform::Delete(p); } p = next; } diff --git a/src/lib/support/Pool.h b/src/lib/support/Pool.h index a2ccd53aed2f60..9bd7d6a5d872b7 100644 --- a/src/lib/support/Pool.h +++ b/src/lib/support/Pool.h @@ -22,6 +22,7 @@ #pragma once +#include #include #include @@ -276,10 +277,10 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon< template T * CreateObject(Args &&... args) { - T * object = new T(std::forward(args)...); + T * object = Platform::New(std::forward(args)...); if (object != nullptr) { - auto node = new internal::HeapObjectListNode(); + auto node = Platform::New(); if (node != nullptr) { node->mObject = object; @@ -300,7 +301,7 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon< { // Note that the node is not removed here; that is deferred until the end of the next pool iteration. node->mObject = nullptr; - delete object; + Platform::Delete(object); DecreaseUsage(); } } @@ -336,14 +337,25 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon< #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP +/** + * Specify ObjectPool storage allocation. + */ enum class ObjectPoolMem { - kStatic, + /** + * Use storage inside the containing scope for both objects and pool management state. + */ + kInline, #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - kDynamic, - kDefault = kDynamic + /** + * Allocate objects from the heap, with only pool management state in the containing scope. + * + * For this case, the ObjectPool size parameter is ignored. + */ + kHeap, + kDefault = kHeap #else // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - kDefault = kStatic + kDefault = kInline #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP }; @@ -351,13 +363,13 @@ template class ObjectPool; template -class ObjectPool : public BitMapObjectPool +class ObjectPool : public BitMapObjectPool { }; #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP template -class ObjectPool : public HeapObjectPool +class ObjectPool : public HeapObjectPool { }; #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP diff --git a/src/lib/support/tests/TestPool.cpp b/src/lib/support/tests/TestPool.cpp index 2d26e260e581db..4870e8f1ede3bd 100644 --- a/src/lib/support/tests/TestPool.cpp +++ b/src/lib/support/tests/TestPool.cpp @@ -62,20 +62,20 @@ void TestReleaseNull(nlTestSuite * inSuite, void * inContext) void TestReleaseNullStatic(nlTestSuite * inSuite, void * inContext) { - TestReleaseNull(inSuite, inContext); + TestReleaseNull(inSuite, inContext); } #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP void TestReleaseNullDynamic(nlTestSuite * inSuite, void * inContext) { - TestReleaseNull(inSuite, inContext); + TestReleaseNull(inSuite, inContext); } #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP template void TestCreateReleaseObject(nlTestSuite * inSuite, void * inContext) { - ObjectPool pool; + ObjectPool pool; uint32_t * obj[N]; NL_TEST_ASSERT(inSuite, pool.Allocated() == 0); @@ -104,9 +104,9 @@ void TestCreateReleaseObject(nlTestSuite * inSuite, void * inContext) void TestCreateReleaseObjectStatic(nlTestSuite * inSuite, void * inContext) { constexpr const size_t kSize = 100; - TestCreateReleaseObject(inSuite, inContext); + TestCreateReleaseObject(inSuite, inContext); - ObjectPool pool; + ObjectPool pool; uint32_t * obj[kSize]; for (size_t i = 0; i < kSize; ++i) @@ -144,7 +144,7 @@ void TestCreateReleaseObjectStatic(nlTestSuite * inSuite, void * inContext) #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP void TestCreateReleaseObjectDynamic(nlTestSuite * inSuite, void * inContext) { - TestCreateReleaseObject(inSuite, inContext); + TestCreateReleaseObject(inSuite, inContext); } #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP @@ -201,13 +201,13 @@ void TestCreateReleaseStruct(nlTestSuite * inSuite, void * inContext) void TestCreateReleaseStructStatic(nlTestSuite * inSuite, void * inContext) { - TestCreateReleaseStruct(inSuite, inContext); + TestCreateReleaseStruct(inSuite, inContext); } #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP void TestCreateReleaseStructDynamic(nlTestSuite * inSuite, void * inContext) { - TestCreateReleaseStruct(inSuite, inContext); + TestCreateReleaseStruct(inSuite, inContext); } #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP @@ -334,13 +334,13 @@ void TestForEachActiveObject(nlTestSuite * inSuite, void * inContext) void TestForEachActiveObjectStatic(nlTestSuite * inSuite, void * inContext) { - TestForEachActiveObject(inSuite, inContext); + TestForEachActiveObject(inSuite, inContext); } #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP void TestForEachActiveObjectDynamic(nlTestSuite * inSuite, void * inContext) { - TestForEachActiveObject(inSuite, inContext); + TestForEachActiveObject(inSuite, inContext); } #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP @@ -397,23 +397,24 @@ void TestPoolInterface(nlTestSuite * inSuite, void * inContext) void TestPoolInterfaceStatic(nlTestSuite * inSuite, void * inContext) { - TestPoolInterface(inSuite, inContext); + TestPoolInterface(inSuite, inContext); } #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP void TestPoolInterfaceDynamic(nlTestSuite * inSuite, void * inContext) { - TestPoolInterface(inSuite, inContext); + TestPoolInterface(inSuite, inContext); } #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP int Setup(void * inContext) { - return SUCCESS; + return ::chip::Platform::MemoryInit() == CHIP_NO_ERROR ? SUCCESS : FAILURE; } int Teardown(void * inContext) { + ::chip::Platform::MemoryShutdown(); return SUCCESS; } diff --git a/src/system/tests/TestSystemTimer.cpp b/src/system/tests/TestSystemTimer.cpp index 93924d2cf5e56f..59199db494605b 100644 --- a/src/system/tests/TestSystemTimer.cpp +++ b/src/system/tests/TestSystemTimer.cpp @@ -442,6 +442,11 @@ static int TestSetup(void * aContext) { TestContext & lContext = *reinterpret_cast(aContext); + if (::chip::Platform::MemoryInit() != CHIP_NO_ERROR) + { + return FAILURE; + } + #if CHIP_SYSTEM_CONFIG_USE_LWIP && LWIP_VERSION_MAJOR == 2 && LWIP_VERSION_MINOR == 0 static sys_mbox_t * sLwIPEventQueue = NULL; @@ -471,6 +476,7 @@ static int TestTeardown(void * aContext) tcpip_finish(NULL, NULL); #endif // CHIP_SYSTEM_CONFIG_USE_LWIP && (LWIP_VERSION_MAJOR == 2) && (LWIP_VERSION_MINOR == 0) + ::chip::Platform::MemoryShutdown(); return (SUCCESS); } diff --git a/src/transport/raw/TCP.h b/src/transport/raw/TCP.h index 3aed361eaa02d8..f6a09bc93f847e 100644 --- a/src/transport/raw/TCP.h +++ b/src/transport/raw/TCP.h @@ -281,7 +281,7 @@ class TCP : public TCPBase private: friend class TCPTest; TCPBase::ActiveConnectionState mConnectionsBuffer[kActiveConnectionsSize]; - PoolImpl mPendingPackets; + PoolImpl mPendingPackets; }; } // namespace Transport