From 3319217d3250ceaf89b6f1488708ebc725d08874 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Fri, 27 Aug 2021 14:38:44 -0700 Subject: [PATCH] Remove APIs which on longer make sense in heap based ObjectPool (#9229) * Remove APIs which do not make sense in dynamic pool * Remove the sMutex protection from unit test --- src/system/SystemObject.h | 62 +----- src/system/WatchableEventManagerLwIP.cpp | 2 +- src/system/tests/TestSystemObject.cpp | 233 ++++------------------- 3 files changed, 34 insertions(+), 263 deletions(-) diff --git a/src/system/SystemObject.h b/src/system/SystemObject.h index b9363e8f90f37f..9a925a2a2f58bd 100644 --- a/src/system/SystemObject.h +++ b/src/system/SystemObject.h @@ -192,9 +192,7 @@ class ObjectPool { public: void Reset(); - size_t Size(); - T * Get(const Layer & aLayer, size_t aIndex); T * TryCreate(Layer & aLayer); void GetStatistics(chip::System::Stats::count_t & aNumInUse, chip::System::Stats::count_t & aHighWatermark); @@ -265,69 +263,11 @@ inline void ObjectPool::Reset() memset(mArena.uMemory, 0, N * sizeof(T)); #if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS - mHighWatermark = 0; + mHighWatermark = 0; #endif #endif } -/** - * @brief - * Returns the number of objects that can be simultaneously retained from a pool. - */ -template -inline size_t ObjectPool::Size() -{ -#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - size_t count = 0; - std::lock_guard lock(mMutex); - Object * p = mDummyHead.mNext; - - while (p) - { - count++; - p = p->mNext; - } - - return count; -#else - return N; -#endif -} - -/** - * @brief - * Returns a pointer the object at \c aIndex or \c NULL if the object is not retained by \c aLayer. - */ -template -inline T * ObjectPool::Get(const Layer & aLayer, size_t aIndex) -{ - T * lReturn = nullptr; - -#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - { - std::lock_guard lock(mMutex); - Object * p = mDummyHead.mNext; - - while (aIndex > 0) - { - if (p == nullptr) - break; - p = p->mNext; - aIndex--; - } - - lReturn = static_cast(p); - } -#else - if (aIndex < N) - lReturn = &reinterpret_cast(mArena.uMemory)[aIndex]; -#endif - - (void) static_cast(lReturn); /* In C++-11, this would be a static_assert that T inherits Object. */ - - return (lReturn != nullptr) && lReturn->IsRetained(aLayer) ? lReturn : nullptr; -} - /** * @brief * Tries to initially retain the first object in the pool that is not retained by any layer. diff --git a/src/system/WatchableEventManagerLwIP.cpp b/src/system/WatchableEventManagerLwIP.cpp index f7da1289ee1781..455c18adb015c6 100644 --- a/src/system/WatchableEventManagerLwIP.cpp +++ b/src/system/WatchableEventManagerLwIP.cpp @@ -278,7 +278,7 @@ CHIP_ERROR WatchableEventManager::HandlePlatformTimer() // The platform timer API has MSEC resolution so expire any timer with less than 1 msec remaining. size_t timersHandled = 0; Timer * timer = nullptr; - while ((timersHandled < Timer::sPool.Size()) && ((timer = mTimerList.PopIfEarlier(currentTime + 1)) != nullptr)) + while ((timersHandled < CHIP_SYSTEM_CONFIG_NUM_TIMERS) && ((timer = mTimerList.PopIfEarlier(currentTime + 1)) != nullptr)) { mHandlingTimerComplete = true; timer->HandleComplete(); diff --git a/src/system/tests/TestSystemObject.cpp b/src/system/tests/TestSystemObject.cpp index a62acff35e94db..7e068368d4aac8 100644 --- a/src/system/tests/TestSystemObject.cpp +++ b/src/system/tests/TestSystemObject.cpp @@ -83,21 +83,19 @@ class TestObject : public Object #endif private: - enum - { - kPoolSize = 112 // a multiple of kNumThreads, less than CHIP_SYS_STATS_COUNT_MAX - }; - static ObjectPool sPool; + static constexpr int kNumThreads = 16; + static constexpr int kLoopIterations = 1000; + static constexpr int kMaxDelayIterations = 3; + static constexpr int kPoolSize = (kNumThreads * 7) + 1; + + static_assert(kNumThreads > 1, "kNumThreads should be more than 1"); static_assert(kPoolSize < CHIP_SYS_STATS_COUNT_MAX, "kPoolSize is not less than CHIP_SYS_STATS_COUNT_MAX"); + static ObjectPool sPool; + #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING unsigned int mDelay; - static constexpr int kNumThreads = 16; - static constexpr int kLoopIterations = 1000; - static constexpr int kMaxDelayIterations = 3; - static_assert(kPoolSize % kNumThreads == 0, "kPoolSize is not a multiple of kNumThreads"); - void Delay(volatile unsigned int & aAccumulator); static void * CheckConcurrencyThread(void * aContext); #if !CHIP_SYSTEM_CONFIG_POOL_USE_HEAP @@ -178,70 +176,11 @@ void TestObject::CheckIteration(nlTestSuite * inSuite, void * aContext) // Test Object retention -#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP -void TestObject::CheckRetention(nlTestSuite * inSuite, void * aContext) -{ - TestContext & lContext = *static_cast(aContext); - Layer lLayer; - unsigned int i, j; - - lLayer.Init(); - sPool.Reset(); - - for (i = 0; i < kPoolSize; ++i) - { - TestObject * lCreated = sPool.TryCreate(lLayer); - - NL_TEST_ASSERT(lContext.mTestSuite, lCreated != nullptr); - NL_TEST_ASSERT(lContext.mTestSuite, lCreated->IsRetained(lLayer)); - NL_TEST_ASSERT(lContext.mTestSuite, &(lCreated->SystemLayer()) == &lLayer); - - lCreated->Init(); - - for (j = 0; j < kPoolSize; ++j) - { - TestObject * lGotten = sPool.Get(lLayer, j); - - if (j > i) - { - NL_TEST_ASSERT(lContext.mTestSuite, lGotten == nullptr); - } - else - { - NL_TEST_ASSERT(lContext.mTestSuite, lGotten != nullptr); - lGotten->Retain(); - } - } - } - - for (i = 0; i < kPoolSize; ++i) - { - TestObject * lGotten = sPool.Get(lLayer, 0); - - NL_TEST_ASSERT(lContext.mTestSuite, lGotten != nullptr); - - for (j = 0; j < i + 1; ++j) - { - NL_TEST_ASSERT(lContext.mTestSuite, lGotten->IsRetained(lLayer)); - lGotten->Release(); - } - - NL_TEST_ASSERT(lContext.mTestSuite, lGotten->IsRetained(lLayer)); - lGotten->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lGotten->IsRetained(lLayer)); - } - - NL_TEST_ASSERT(lContext.mTestSuite, sPool.Size() == 0); - lLayer.Shutdown(); -} - -#else // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - void TestObject::CheckRetention(nlTestSuite * inSuite, void * aContext) { TestContext & lContext = *static_cast(aContext); Layer lLayer; - unsigned int i, j; + unsigned int i; lLayer.Init(); sPool.Reset(); @@ -251,54 +190,28 @@ void TestObject::CheckRetention(nlTestSuite * inSuite, void * aContext) TestObject * lCreated = sPool.TryCreate(lLayer); NL_TEST_ASSERT(lContext.mTestSuite, lCreated != nullptr); - NL_TEST_ASSERT(lContext.mTestSuite, lCreated->IsRetained(lLayer)); - NL_TEST_ASSERT(lContext.mTestSuite, &(lCreated->SystemLayer()) == &lLayer); lCreated->Init(); - - for (j = 0; j < kPoolSize; ++j) - { - TestObject * lGotten = sPool.Get(lLayer, j); - - if (j > i) - { - NL_TEST_ASSERT(lContext.mTestSuite, lGotten == nullptr); - } - else - { - NL_TEST_ASSERT(lContext.mTestSuite, lGotten != nullptr); - lGotten->Retain(); - } - } } - for (i = 0; i < kPoolSize; ++i) - { - TestObject * lGotten = sPool.Get(lLayer, i); - - NL_TEST_ASSERT(lContext.mTestSuite, lGotten != nullptr); - - for (j = kPoolSize; j > i; --j) - { - NL_TEST_ASSERT(lContext.mTestSuite, lGotten->IsRetained(lLayer)); - lGotten->Release(); - } + i = 0; + TestObject::sPool.ForEachActiveObject([&](TestObject * lGotten) { + lGotten->Retain(); + i++; + return true; + }); + NL_TEST_ASSERT(lContext.mTestSuite, i == kPoolSize); - NL_TEST_ASSERT(lContext.mTestSuite, lGotten->IsRetained(lLayer)); + i = 0; + TestObject::sPool.ForEachActiveObject([&](TestObject * lGotten) { lGotten->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lGotten->IsRetained(lLayer)); - } - - for (i = 0; i < kPoolSize; ++i) - { - TestObject * lGotten = sPool.Get(lLayer, i); - - NL_TEST_ASSERT(lContext.mTestSuite, lGotten == nullptr); - } + i++; + return true; + }); + NL_TEST_ASSERT(lContext.mTestSuite, i == kPoolSize); lLayer.Shutdown(); } -#endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP // Test Object concurrency @@ -347,24 +260,10 @@ void * TestObject::CheckConcurrencyThread(void * aContext) lObject->Delay(lContext.mAccumulator); } - // Free the last object of the pool, if it belongs to - // this thread. - - lObject = sPool.Get(lLayer, kPoolSize - 1); - - if (lObject != nullptr) - { - lObject->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lObject->IsRetained(lLayer)); - } - - // For each iteration, take one more object, and free one starting from the end - // of the pool + // For each iteration, take one more object, and free it form the pool for (i = 0; i < kLoopIterations; ++i) { - unsigned long int j; - lObject = nullptr; while (lObject == nullptr) { @@ -377,36 +276,7 @@ void * TestObject::CheckConcurrencyThread(void * aContext) lObject->Init(); lObject->Delay(lContext.mAccumulator); -#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP - j = sPool.Size(); -#else - j = kPoolSize; -#endif - lObject = nullptr; - while (j-- > 0) - { - lObject = sPool.Get(lLayer, j); - - if (lObject == nullptr) - continue; - - lObject->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lObject->IsRetained(lLayer)); - break; - } - } - - // Cleanup - - for (i = 0; i < kPoolSize; ++i) - { - lObject = sPool.Get(lLayer, i); - - if (lObject == nullptr) - continue; - lObject->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lObject->IsRetained(lLayer)); } lLayer.Shutdown(); @@ -471,6 +341,8 @@ void TestObject::CheckConcurrency(nlTestSuite * inSuite, void * aContext) #if !CHIP_SYSTEM_CONFIG_POOL_USE_HEAP void TestObject::CheckHighWatermarkConcurrency(nlTestSuite * inSuite, void * aContext) { + sPool.Reset(); + #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING for (unsigned int i = 0; i < 100; i++) { @@ -494,7 +366,6 @@ void TestObject::CheckHighWatermark(nlTestSuite * inSuite, void * aContext) // Take all objects one at a time and check the watermark // increases monotonically - for (int i = 0; i < kNumObjects; ++i) { lObject = sPool.TryCreate(lLayer); @@ -510,60 +381,20 @@ void TestObject::CheckHighWatermark(nlTestSuite * inSuite, void * aContext) } // Fail an allocation and check that both stats don't change - - lObject = sPool.TryCreate(lLayer); - NL_TEST_ASSERT(lContext.mTestSuite, lObject == nullptr); + NL_TEST_ASSERT(lContext.mTestSuite, sPool.TryCreate(lLayer) == nullptr); sPool.GetStatistics(lNumInUse, lHighWatermark); NL_TEST_ASSERT(lContext.mTestSuite, lNumInUse == kNumObjects); NL_TEST_ASSERT(lContext.mTestSuite, lHighWatermark == kNumObjects); - // Free all objects one at a time and check that the watermark does not + // Free the last object and check that the watermark does not // change. + lObject->Release(); + NL_TEST_ASSERT(lContext.mTestSuite, !lObject->IsRetained(lLayer)); - for (int i = 0; i < kNumObjects; ++i) - { - lObject = sPool.Get(lLayer, static_cast(i)); - - NL_TEST_ASSERT(lContext.mTestSuite, lObject != nullptr); - - lObject->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lObject->IsRetained(lLayer)); - - sPool.GetStatistics(lNumInUse, lHighWatermark); - NL_TEST_ASSERT(lContext.mTestSuite, lNumInUse == (kNumObjects - i - 1)); - NL_TEST_ASSERT(lContext.mTestSuite, lHighWatermark == kNumObjects); - } - - // Take all objects one at a time again and check the watermark - // does not move - - for (int i = 0; i < kNumObjects; ++i) - { - lObject = sPool.TryCreate(lLayer); - - NL_TEST_ASSERT(lContext.mTestSuite, lObject->IsRetained(lLayer)); - NL_TEST_ASSERT(lContext.mTestSuite, &(lObject->SystemLayer()) == &lLayer); - - sPool.GetStatistics(lNumInUse, lHighWatermark); - NL_TEST_ASSERT(lContext.mTestSuite, lNumInUse == (i + 1)); - NL_TEST_ASSERT(lContext.mTestSuite, lHighWatermark == kNumObjects); - - lObject->Init(); - } - - // Cleanup - - for (size_t i = 0; i < kPoolSize; ++i) - { - lObject = sPool.Get(lLayer, i); - - if (lObject == nullptr) - continue; - - lObject->Release(); - NL_TEST_ASSERT(lContext.mTestSuite, !lObject->IsRetained(lLayer)); - } + sPool.GetStatistics(lNumInUse, lHighWatermark); + NL_TEST_ASSERT(lContext.mTestSuite, lNumInUse == (kNumObjects - 1)); + NL_TEST_ASSERT(lContext.mTestSuite, lHighWatermark == kNumObjects); lLayer.Shutdown(); }