From abe53f45554d759c20300260ee3794237fac2b71 Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Fri, 29 Apr 2022 15:15:36 -0700 Subject: [PATCH] Revert "[IM] Merge paths when global dirty set is exhausted (#17417)" This reverts commit 300546290f88f4151be1bbc6111103aebc1482b5. --- src/app/AttributePathParams.h | 13 --- src/app/reporting/Engine.cpp | 124 ++-------------------- src/app/reporting/Engine.h | 30 ------ src/app/tests/TestReportingEngine.cpp | 144 -------------------------- src/lib/support/Pool.h | 5 - 5 files changed, 10 insertions(+), 306 deletions(-) diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h index 47ac890f8de3c3..54803bf9bed828 100644 --- a/src/app/AttributePathParams.h +++ b/src/app/AttributePathParams.h @@ -51,12 +51,6 @@ struct AttributePathParams bool IsWildcardPath() const { return HasWildcardEndpointId() || HasWildcardClusterId() || HasWildcardAttributeId(); } - bool operator==(const AttributePathParams & aOther) const - { - return mEndpointId == aOther.mEndpointId && mClusterId == aOther.mClusterId && mAttributeId == aOther.mAttributeId && - mListIndex == aOther.mListIndex; - } - /** * SPEC 8.9.2.2 * Check that the path meets some basic constraints of an attribute path: If list index is not wildcard, then field id must not @@ -69,13 +63,6 @@ struct AttributePathParams inline bool HasWildcardClusterId() const { return mClusterId == kInvalidClusterId; } inline bool HasWildcardAttributeId() const { return mAttributeId == kInvalidAttributeId; } inline bool HasWildcardListIndex() const { return mListIndex == kInvalidListIndex; } - inline void SetWildcardEndpointId() { mEndpointId = kInvalidEndpointId; } - inline void SetWildcardClusterId() { mClusterId = kInvalidClusterId; } - inline void SetWildcardAttributeId() - { - mAttributeId = kInvalidAttributeId; - mListIndex = kInvalidListIndex; - } bool IsAttributePathSupersetOf(const AttributePathParams & other) const { diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index f9bd3fb9439422..448f0b44a282a1 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -658,116 +658,6 @@ bool Engine::MergeOverlappedAttributePath(const AttributePathParams & aAttribute }); } -bool Engine::ClearTombPaths() -{ - bool pathReleased = false; - mGlobalDirtySet.ForEachActiveObject([&](auto * path) { - if (path->mGeneration == 0) - { - mGlobalDirtySet.ReleaseObject(path); - pathReleased = true; - } - return Loop::Continue; - }); - return pathReleased; -} - -bool Engine::MergeDirtyPathsUnderSameCluster() -{ - mGlobalDirtySet.ForEachActiveObject([&](auto * outerPath) { - if (outerPath->HasWildcardClusterId() || outerPath->mGeneration == 0) - { - return Loop::Continue; - } - mGlobalDirtySet.ForEachActiveObject([&](auto * innerPath) { - if (innerPath == outerPath) - { - return Loop::Continue; - } - // We don't support paths with a wildcard endpoint + a concrete cluster in global dirty set, so we do a simple == check - // here. - if (innerPath->mEndpointId != outerPath->mEndpointId || innerPath->mClusterId != outerPath->mClusterId) - { - return Loop::Continue; - } - if (innerPath->mGeneration > outerPath->mGeneration) - { - outerPath->mGeneration = innerPath->mGeneration; - } - outerPath->SetWildcardAttributeId(); - - // The object pool does not allow us to release objects in a nested iteration, mark the path as a tomb by setting its - // generation to 0 and then clear it later. - innerPath->mGeneration = 0; - return Loop::Continue; - }); - return Loop::Continue; - }); - - return ClearTombPaths(); -} - -bool Engine::MergeDirtyPathsUnderSameEndpoint() -{ - mGlobalDirtySet.ForEachActiveObject([&](auto * outerPath) { - if (outerPath->HasWildcardEndpointId() || outerPath->mGeneration == 0) - { - return Loop::Continue; - } - mGlobalDirtySet.ForEachActiveObject([&](auto * innerPath) { - if (innerPath == outerPath) - { - return Loop::Continue; - } - if (innerPath->mEndpointId != outerPath->mEndpointId) - { - return Loop::Continue; - } - if (innerPath->mGeneration > outerPath->mGeneration) - { - outerPath->mGeneration = innerPath->mGeneration; - } - outerPath->SetWildcardClusterId(); - outerPath->SetWildcardAttributeId(); - - // The object pool does not allow us to release objects in a nested iteration, mark the path as a tomb by setting its - // generation to 0 and then clear it later. - innerPath->mGeneration = 0; - return Loop::Continue; - }); - return Loop::Continue; - }); - return ClearTombPaths(); -} - -CHIP_ERROR Engine::InsertPathIntoDirtySet(const AttributePathParams & aAttributePath) -{ - ReturnErrorCodeIf(MergeOverlappedAttributePath(aAttributePath), CHIP_NO_ERROR); - - if (mGlobalDirtySet.Exhausted() && !MergeDirtyPathsUnderSameCluster() && !MergeDirtyPathsUnderSameEndpoint()) - { - ChipLogDetail(DataManagement, "Global dirty set pool exhausted, merge all paths."); - mGlobalDirtySet.ReleaseAll(); - auto object = mGlobalDirtySet.CreateObject(); - object->mGeneration = GetDirtySetGeneration(); - } - - ReturnErrorCodeIf(MergeOverlappedAttributePath(aAttributePath), CHIP_NO_ERROR); - ChipLogDetail(DataManagement, "Cannot merge the new path into any existing path, create one."); - - auto object = mGlobalDirtySet.CreateObject(); - if (object == nullptr) - { - // This should not happen, this path should be merged into the wildcard endpoint at least. - ChipLogError(DataManagement, "mGlobalDirtySet pool full, cannot handle more entries!"); - return CHIP_ERROR_NO_MEMORY; - } - *object = aAttributePath; - object->mGeneration = GetDirtySetGeneration(); - - return CHIP_NO_ERROR; -} - CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) { BumpDirtySetGeneration(); @@ -792,13 +682,19 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) return Loop::Continue; }); - if (!InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aAttributePath)) + if (!MergeOverlappedAttributePath(aAttributePath) && + InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aAttributePath)) { - return CHIP_NO_ERROR; + auto object = mGlobalDirtySet.CreateObject(); + if (object == nullptr) + { + ChipLogError(DataManagement, "mGlobalDirtySet pool full, cannot handle more entries!"); + return CHIP_ERROR_NO_MEMORY; + } + *object = aAttributePath; + object->mGeneration = GetDirtySetGeneration(); } - ReturnErrorOnFailure(InsertPathIntoDirtySet(aAttributePath)); - // Schedule work to run asynchronously on the CHIP thread. The scheduled // work won't execute until the current execution context has // completed. This ensures that we can 'gather up' multiple attribute diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h index 45d9441429ced5..4e4c78ae24cf29 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -188,31 +188,6 @@ class Engine */ bool MergeOverlappedAttributePath(const AttributePathParams & aAttributePath); - /** - * If we are running out of ObjectPool for the global dirty set, we will try to merge the existing items by clusters. - * - * Returns whether we have released any paths. - */ - bool MergeDirtyPathsUnderSameCluster(); - - /** - * If we are running out of ObjectPool for the global dirty set and we cannot find a slot after merging the existing items by - * clusters, we will try to merge the existing items by endpoints. - * - * Returns whether we have released any paths. - */ - bool MergeDirtyPathsUnderSameEndpoint(); - - /** - * During the iterating of the paths, releasing the object in the inner loop will cause undefined behavior of the ObjectPool, so - * we replace the items to be cleared by a tomb first, then clear all the tombs after the iteration. - * - * Returns whether we have released any paths. - */ - bool ClearTombPaths(); - - CHIP_ERROR InsertPathIntoDirtySet(const AttributePathParams & aAttributePath); - inline void BumpDirtySetGeneration() { mDirtyGeneration++; } /** @@ -243,12 +218,7 @@ class Engine * mGlobalDirtySet is used to track the set of attribute/event paths marked dirty for reporting purposes. * */ -#if CONFIG_IM_BUILD_FOR_UNIT_TEST - // For unit tests, always use inline allocation for code coverage. - ObjectPool mGlobalDirtySet; -#else ObjectPool mGlobalDirtySet; -#endif /** * A generation counter for the dirty attrbute set. diff --git a/src/app/tests/TestReportingEngine.cpp b/src/app/tests/TestReportingEngine.cpp index 2a59758f0f9ba5..aba514ac4bebaa 100644 --- a/src/app/tests/TestReportingEngine.cpp +++ b/src/app/tests/TestReportingEngine.cpp @@ -35,7 +35,6 @@ #include #include -#include #include using TestContext = chip::Test::AppContext; @@ -54,54 +53,6 @@ class TestReportingEngine public: static void TestBuildAndSendSingleReportData(nlTestSuite * apSuite, void * apContext); static void TestMergeOverlappedAttributePath(nlTestSuite * apSuite, void * apContext); - static void TestMergeAttributePathWhenDirtySetPoolExhausted(nlTestSuite * apSuite, void * apContext); - -private: - static bool InsertToDirtySet(const AttributePathParams & aPath); - - struct ExpectedDirtySetContent : public AttributePathParams - { - ExpectedDirtySetContent(const AttributePathParams & path) : AttributePathParams(path) {} - bool verified = false; - }; - - template - static bool VerifyDirtySetContent(const Args &... args) - { - const int size = sizeof...(args); - ExpectedDirtySetContent content[size] = { ExpectedDirtySetContent(args)... }; - - if (InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ForEachActiveObject([&](auto * path) { - for (int i = 0; i < size; i++) - { - if (static_cast(content[i]) == static_cast(*path)) - { - content[i].verified = true; - return Loop::Continue; - } - } - ChipLogDetail(DataManagement, - "Dirty path Endpoint %" PRIx16 " Cluster %" PRIx32 ", Attribute %" PRIx32 " is not expected", - uint16_t(path->mEndpointId), path->mClusterId, path->mAttributeId); - return Loop::Break; - }) == Loop::Break) - { - return false; - } - - for (int i = 0; i < size; i++) - { - if (!content[i].verified) - { - ChipLogDetail(DataManagement, - "Dirty path Endpoint %" PRIx16 " Cluster %" PRIx32 ", Attribute %" PRIx32 - " is not found in the dirty set", - uint16_t(content[i].mEndpointId), content[i].mClusterId, content[i].mAttributeId); - return false; - } - } - return true; - } }; class TestExchangeDelegate : public Messaging::ExchangeDelegate @@ -225,100 +176,6 @@ void TestReportingEngine::TestMergeOverlappedAttributePath(nlTestSuite * apSuite InteractionModelEngine::GetInstance()->GetReportingEngine().Shutdown(); } -bool TestReportingEngine::InsertToDirtySet(const AttributePathParams & aPath) -{ - auto path = InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.CreateObject(); - VerifyOrReturnError(path != nullptr, false); - *path = aPath; - path->mGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); - return true; -} - -void TestReportingEngine::TestMergeAttributePathWhenDirtySetPoolExhausted(nlTestSuite * apSuite, void * apContext) -{ - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; - err = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager()); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - - InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll(); - InteractionModelEngine::GetInstance()->GetReportingEngine().BumpDirtySetGeneration(); - - // Case 1: All dirty paths including the new one are under the same cluster. - // -> Expected behavior: The dirty set is replaced by a wildcard attribute path under the same cluster. - for (AttributeId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++) - { - NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, kTestClusterId, i))); - } - NL_TEST_ASSERT(apSuite, - CHIP_NO_ERROR == - InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet( - AttributePathParams(kTestEndpointId, kTestClusterId, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET + 1))); - NL_TEST_ASSERT(apSuite, VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kTestClusterId))); - - InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll(); - - // Case 2: All dirty paths including the new one are under the same endpoint. - // -> Expected behavior: The dirty set is replaced by a wildcard cluster path under the same endpoint. - for (ClusterId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++) - { - NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, i, 1))); - } - NL_TEST_ASSERT(apSuite, - CHIP_NO_ERROR == - InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet( - AttributePathParams(kTestEndpointId, ClusterId(CHIP_IM_SERVER_MAX_NUM_DIRTY_SET + 1), 1))); - NL_TEST_ASSERT(apSuite, VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kInvalidClusterId))); - - InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll(); - - // Case 3: All dirty paths including the new one are under the different endpoints. - // -> Expected behavior: The dirty set is replaced by a wildcard endpoint. - for (EndpointId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++) - { - NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(EndpointId(i), i, i))); - } - NL_TEST_ASSERT(apSuite, - CHIP_NO_ERROR == - InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet( - AttributePathParams(EndpointId(CHIP_IM_SERVER_MAX_NUM_DIRTY_SET + 1), 1, 1))); - NL_TEST_ASSERT(apSuite, VerifyDirtySetContent(AttributePathParams())); - - InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll(); - - // Case 4: All existing dirty paths are under the same cluster, the new path comes from another cluster. - // -> Expected behavior: The existing paths are merged into one single wildcard attribute path. New path is inserted as-is. - for (EndpointId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++) - { - NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, kTestClusterId, i))); - } - NL_TEST_ASSERT(apSuite, - CHIP_NO_ERROR == - InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet( - AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1))); - NL_TEST_ASSERT(apSuite, - VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kTestClusterId), - AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1))); - - InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll(); - - // Case 5: All existing dirty paths are under the same endpoint, the new path comes from another endpoint. - // -> Expected behavior: The existing paths are merged into one single wildcard cluster path. New path is inserted as-is. - for (EndpointId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++) - { - NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, i, 1))); - } - NL_TEST_ASSERT(apSuite, - CHIP_NO_ERROR == - InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet( - AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1))); - NL_TEST_ASSERT(apSuite, - VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kInvalidClusterId), - AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1))); - - InteractionModelEngine::GetInstance()->GetReportingEngine().Shutdown(); -} - } // namespace reporting } // namespace app } // namespace chip @@ -329,7 +186,6 @@ const nlTest sTests[] = { NL_TEST_DEF("CheckBuildAndSendSingleReportData", chip::app::reporting::TestReportingEngine::TestBuildAndSendSingleReportData), NL_TEST_DEF("TestMergeOverlappedAttributePath", chip::app::reporting::TestReportingEngine::TestMergeOverlappedAttributePath), - NL_TEST_DEF("TestMergeAttributePathWhenDirtySetPoolExhausted", chip::app::reporting::TestReportingEngine::TestMergeAttributePathWhenDirtySetPoolExhausted), NL_TEST_SENTINEL() }; // clang-format on diff --git a/src/lib/support/Pool.h b/src/lib/support/Pool.h index 5266d05680239a..aa0f2e76e7c425 100644 --- a/src/lib/support/Pool.h +++ b/src/lib/support/Pool.h @@ -336,11 +336,6 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon< */ size_t Capacity() const { return SIZE_MAX; } - /* - * This method exists purely to line up with the static allocator version. Heap based object pool will never be exhausted. - */ - bool Exhausted() const { return false; } - void ReleaseObject(T * object) { if (object != nullptr)