From 9aeecb35eeb088b8a992797cfa71533bb897288c Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Wed, 4 May 2022 11:53:32 -0700 Subject: [PATCH] Un-revert #17417 (#17974) * [IM] Merge paths when global dirty set is exhausted (#17417) * [IM] Merge paths when global dirty set is exhausted * Address comments * Fix build * Compile breakage fix * Apply suggestions from code review Co-authored-by: Boris Zbarsky Co-authored-by: Song GUO Co-authored-by: Boris Zbarsky --- src/app/AttributePathParams.h | 13 +++ src/app/reporting/Engine.cpp | 124 ++++++++++++++++++++-- src/app/reporting/Engine.h | 30 ++++++ src/app/tests/TestReportingEngine.cpp | 142 ++++++++++++++++++++++++++ src/lib/support/Pool.h | 5 + 5 files changed, 304 insertions(+), 10 deletions(-) diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h index 54803bf9bed828..47ac890f8de3c3 100644 --- a/src/app/AttributePathParams.h +++ b/src/app/AttributePathParams.h @@ -51,6 +51,12 @@ 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 @@ -63,6 +69,13 @@ 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 448f0b44a282a1..f9bd3fb9439422 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -658,6 +658,116 @@ 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(); @@ -682,19 +792,13 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) return Loop::Continue; }); - if (!MergeOverlappedAttributePath(aAttributePath) && - InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aAttributePath)) + if (!InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aAttributePath)) { - 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(); + return CHIP_NO_ERROR; } + 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 4e4c78ae24cf29..45d9441429ced5 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -188,6 +188,31 @@ 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++; } /** @@ -218,7 +243,12 @@ 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 aba514ac4bebaa..6e66d15879a6de 100644 --- a/src/app/tests/TestReportingEngine.cpp +++ b/src/app/tests/TestReportingEngine.cpp @@ -35,6 +35,7 @@ #include #include +#include #include using TestContext = chip::Test::AppContext; @@ -53,6 +54,52 @@ 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 %x Cluster %" PRIx32 ", Attribute %" PRIx32 " is not expected", + 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 %x Cluster %" PRIx32 ", Attribute %" PRIx32 " is not found in the dirty set", + content[i].mEndpointId, content[i].mClusterId, content[i].mAttributeId); + return false; + } + } + return true; + } }; class TestExchangeDelegate : public Messaging::ExchangeDelegate @@ -176,6 +223,100 @@ 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(), &ctx.GetFabricTable()); + 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 @@ -186,6 +327,7 @@ 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 c2f7958f1e3908..f1f7b0dd243c14 100644 --- a/src/lib/support/Pool.h +++ b/src/lib/support/Pool.h @@ -343,6 +343,11 @@ 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)