[IM] Merge paths when global dirty set is exhausted

src/app/AttributePathParams.h

@@ -51,6 +51,12 @@ struct AttributePathParams
 
     bool HasAttributeWildcard() 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
     {

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

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<AttributePathParamsWithGeneration, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET, ObjectPoolMem::kInline> mGlobalDirtySet;
+#else
     ObjectPool<AttributePathParamsWithGeneration, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET> mGlobalDirtySet;
+#endif
 
     /**
      * A generation counter for the dirty attrbute set.

src/app/tests/TestReportingEngine.cpp

@@ -35,6 +35,7 @@
 #include <messaging/ExchangeContext.h>
 #include <messaging/Flags.h>
 
+#include <cinttypes>
 #include <nlunit-test.h>
 
 using TestContext = chip::Test::AppContext;
@@ -53,6 +54,54 @@ 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 <typename... Args>
+    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<AttributePathParams>(content[i]) == static_cast<AttributePathParams>(*path))
+                    {
+                        content[i].verified = true;
+                        return Loop::Continue;
+                    }
+                }
+                ChipLogDetail(DataManagement,
+                              "Dirty path Endpoint %" PRIx8 " 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 %" PRIx8 " 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 +225,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<TestContext *>(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
@@ -186,6 +329,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