Split global dirty set into dedicated pool (#13472)

project-chip · Aug 8, 2023 · 4503385 · 4503385
1 parent dea34a3
commit 4503385
Show file tree

Hide file tree

Showing 7 changed files with 101 additions and 107 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -46,13 +46,6 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM
 
     mReportingEngine.Init();
 
-    for (uint32_t index = 0; index < CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS - 1; index++)
-    {
-        mClusterInfoPool[index].mpNext = &mClusterInfoPool[index + 1];
-    }
-    mClusterInfoPool[CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS - 1].mpNext = nullptr;
-    mpNextAvailableClusterInfo                                      = mClusterInfoPool;
-
     mMagic++;
 
     return CHIP_NO_ERROR;
@@ -132,12 +125,7 @@ void InteractionModelEngine::Shutdown()
 
     mReportingEngine.Shutdown();
 
-    for (uint32_t index = 0; index < CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS; index++)
-    {
-        mClusterInfoPool[index].mpNext = nullptr;
-    }
-
-    mpNextAvailableClusterInfo = nullptr;
+    mClusterInfoPool.ReleaseAll();
 
     mpExchangeMgr->UnregisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id);
 }
@@ -515,59 +503,31 @@ bool InteractionModelEngine::InActiveReadClientList(ReadClient * apReadClient)
 
 void InteractionModelEngine::ReleaseClusterInfoList(ClusterInfo *& aClusterInfo)
 {
-    ClusterInfo * lastClusterInfo = aClusterInfo;
-    if (lastClusterInfo == nullptr)
+    ClusterInfo * current = aClusterInfo;
+    while (current != nullptr)
     {
-        return;
+        ClusterInfo * next = current->mpNext;
+        mClusterInfoPool.ReleaseObject(current);
+        current = next;
     }
 
-    while (lastClusterInfo != nullptr && lastClusterInfo->mpNext != nullptr)
-    {
-        lastClusterInfo = lastClusterInfo->mpNext;
-    }
-    lastClusterInfo->mpNext    = mpNextAvailableClusterInfo;
-    mpNextAvailableClusterInfo = aClusterInfo;
-    aClusterInfo               = nullptr;
+    aClusterInfo = nullptr;
 }
 
 CHIP_ERROR InteractionModelEngine::PushFront(ClusterInfo *& aClusterInfoList, ClusterInfo & aClusterInfo)
 {
-    ClusterInfo * last = aClusterInfoList;
-    if (mpNextAvailableClusterInfo == nullptr)
+    ClusterInfo * clusterInfo = mClusterInfoPool.CreateObject();
+    if (clusterInfo == nullptr)
     {
         ChipLogError(InteractionModel, "ClusterInfo pool full, cannot handle more entries!");
         return CHIP_ERROR_NO_MEMORY;
     }
-    aClusterInfoList           = mpNextAvailableClusterInfo;
-    mpNextAvailableClusterInfo = mpNextAvailableClusterInfo->mpNext;
-    *aClusterInfoList          = aClusterInfo;
-    aClusterInfoList->mpNext   = last;
+    *clusterInfo        = aClusterInfo;
+    clusterInfo->mpNext = aClusterInfoList;
+    aClusterInfoList    = clusterInfo;
     return CHIP_NO_ERROR;
 }
 
-bool InteractionModelEngine::MergeOverlappedAttributePath(ClusterInfo * apAttributePathList, ClusterInfo & aAttributePath)
-{
-    ClusterInfo * runner = apAttributePathList;
-    while (runner != nullptr)
-    {
-        // If overlapped, we would skip this target path,
-        // --If targetPath is part of previous path, return true
-        // --If previous path is part of target path, update filedid and listindex and mflags with target path, return true
-        if (runner->IsAttributePathSupersetOf(aAttributePath))
-        {
-            return true;
-        }
-        if (aAttributePath.IsAttributePathSupersetOf(*runner))
-        {
-            runner->mListIndex   = aAttributePath.mListIndex;
-            runner->mAttributeId = aAttributePath.mAttributeId;
-            return true;
-        }
-        runner = runner->mpNext;
-    }
-    return false;
-}
-
 bool InteractionModelEngine::IsOverlappedAttributePath(ClusterInfo & aAttributePath)
 {
     for (auto & handler : mReadHandlers)

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -144,9 +144,6 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
 
     void ReleaseClusterInfoList(ClusterInfo *& aClusterInfo);
     CHIP_ERROR PushFront(ClusterInfo *& aClusterInfoLisst, ClusterInfo & aClusterInfo);
-    // Merges aAttributePath inside apAttributePathList if current path is overlapped with existing path in apAttributePathList
-    // Overlap means the path is superset or subset of another path
-    bool MergeOverlappedAttributePath(ClusterInfo * apAttributePathList, ClusterInfo & aAttributePath);
     bool IsOverlappedAttributePath(ClusterInfo & aAttributePath);
 
     CHIP_ERROR RegisterCommandHandler(CommandHandlerInterface * handler);
@@ -267,8 +264,7 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
     WriteClient mWriteClients[CHIP_IM_MAX_NUM_WRITE_CLIENT];
     WriteHandler mWriteHandlers[CHIP_IM_MAX_NUM_WRITE_HANDLER];
     reporting::Engine mReportingEngine;
-    ClusterInfo mClusterInfoPool[CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS];
-    ClusterInfo * mpNextAvailableClusterInfo = nullptr;
+    BitMapObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS> mClusterInfoPool;
 
     ReadClient * mpActiveReadClientList = nullptr;
 

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -44,8 +44,7 @@ void Engine::Shutdown()
 {
     mNumReportsInFlight = 0;
     mCurReadHandlerIdx  = 0;
-    InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpGlobalDirtySet);
-    mpGlobalDirtySet = nullptr;
+    mGlobalDirtySet.ReleaseAll();
 }
 
 CHIP_ERROR
@@ -96,14 +95,14 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
             {
                 bool concretePathDirty = false;
                 // TODO: Optimize this implementation by making the iterator only emit intersected paths.
-                for (auto dirtyPath = mpGlobalDirtySet; dirtyPath != nullptr; dirtyPath = dirtyPath->mpNext)
-                {
+                mGlobalDirtySet.ForEachActiveObject([&](auto * dirtyPath) {
                     if (dirtyPath->IsAttributePathSupersetOf(readPath))
                     {
                         concretePathDirty = true;
-                        break;
+                        return Loop::Break;
                     }
-                }
+                    return Loop::Continue;
+                });
 
                 if (!concretePathDirty)
                 {
@@ -489,10 +488,27 @@ void Engine::Run()
 
     if (allReadClean)
     {
-        InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpGlobalDirtySet);
+        mGlobalDirtySet.ReleaseAll();
     }
 }
 
+bool Engine::MergeOverlappedAttributePath(ClusterInfo & aAttributePath)
+{
+    return Loop::Break == mGlobalDirtySet.ForEachActiveObject([&](auto * path) {
+        if (path->IsAttributePathSupersetOf(aAttributePath))
+        {
+            return Loop::Break;
+        }
+        if (aAttributePath.IsAttributePathSupersetOf(*path))
+        {
+            path->mListIndex   = aAttributePath.mListIndex;
+            path->mAttributeId = aAttributePath.mAttributeId;
+            return Loop::Break;
+        }
+        return Loop::Continue;
+    });
+}
+
 CHIP_ERROR Engine::SetDirty(ClusterInfo & aClusterInfo)
 {
     for (auto & handler : InteractionModelEngine::GetInstance()->mReadHandlers)
@@ -513,10 +529,16 @@ CHIP_ERROR Engine::SetDirty(ClusterInfo & aClusterInfo)
             }
         }
     }
-    if (!InteractionModelEngine::GetInstance()->MergeOverlappedAttributePath(mpGlobalDirtySet, aClusterInfo) &&
+    if (!MergeOverlappedAttributePath(aClusterInfo) &&
         InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aClusterInfo))
     {
-        ReturnLogErrorOnFailure(InteractionModelEngine::GetInstance()->PushFront(mpGlobalDirtySet, aClusterInfo));
+        ClusterInfo * clusterInfo = mGlobalDirtySet.CreateObject();
+        if (clusterInfo == nullptr)
+        {
+            ChipLogError(DataManagement, "mGlobalDirtySet pool full, cannot handle more entries!");
+            return CHIP_ERROR_NO_MEMORY;
+        }
+        *clusterInfo = aClusterInfo;
     }
     return CHIP_NO_ERROR;
 }
@@ -535,17 +557,22 @@ void Engine::UpdateReadHandlerDirty(ReadHandler & aReadHandler)
     bool intersected = false;
     for (auto clusterInfo = aReadHandler.GetAttributeClusterInfolist(); clusterInfo != nullptr; clusterInfo = clusterInfo->mpNext)
     {
-        for (auto path = mpGlobalDirtySet; path != nullptr; path = path->mpNext)
-        {
+        mGlobalDirtySet.ForEachActiveObject([&](auto * path) {
             if (path->IsAttributePathSupersetOf(*clusterInfo) || clusterInfo->IsAttributePathSupersetOf(*path))
             {
                 intersected = true;
-                break;
+                return Loop::Break;
             }
+            return Loop::Continue;
+        });
+        if (intersected)
+        {
+            break;
         }
     }
     if (!intersected)
     {
+        ChipLogDetail(InteractionModel, "clear read handler dirty in UpdateReadHandlerDirty!");
         aReadHandler.ClearDirty();
     }
 }

diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h
@@ -135,6 +135,14 @@ class Engine
     CHIP_ERROR ScheduleBufferPressureEventDelivery(uint32_t aBytesWritten);
     void GetMinEventLogPosition(uint32_t & aMinLogPosition);
 
+    /**
+     * If the provided path is a superset of our of our existing paths, update that existing path to match the
+     * provided path.
+     *
+     * Return whether one of our paths is now a superset of the provided path.
+     */
+    bool MergeOverlappedAttributePath(ClusterInfo & aAttributePath);
+
     /**
      * Boolean to indicate if ScheduleRun is pending. This flag is used to prevent calling ScheduleRun multiple times
      * within the same execution context to avoid applying too much pressure on platforms that use small, fixed size event queues.
@@ -155,13 +163,10 @@ class Engine
     uint32_t mCurReadHandlerIdx = 0;
 
     /**
-     *  mpGlobalDirtySet is used to track the dirty cluster info application modified for attributes during
-     *  post-subscription via SetDirty API, and further form the report. This reporting engine acquires this global dirty
-     *  set from mClusterInfoPool managed by InteractionModelEngine, where all active read handlers also acquire the interested
-     *  cluster Info list from mClusterInfoPool.
+     *  mGlobalDirtySet is used to track the set of attribute/event paths marked dirty for reporting purposes.
      *
      */
-    ClusterInfo * mpGlobalDirtySet = nullptr;
+    BitMapObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET> mGlobalDirtySet;
 
 #if CONFIG_IM_BUILD_FOR_UNIT_TEST
     uint32_t mReservedSize = 0;

diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp
@@ -44,7 +44,6 @@ class TestInteractionModelEngine
 {
 public:
     static void TestClusterInfoPushRelease(nlTestSuite * apSuite, void * apContext);
-    static void TestMergeOverlappedAttributePath(nlTestSuite * apSuite, void * apContext);
     static int GetClusterInfoListLength(ClusterInfo * apClusterInfoList);
 };
 
@@ -90,37 +89,6 @@ void TestInteractionModelEngine::TestClusterInfoPushRelease(nlTestSuite * apSuit
     InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(clusterInfoList);
     NL_TEST_ASSERT(apSuite, GetClusterInfoListLength(clusterInfoList) == 0);
 }
-
-void TestInteractionModelEngine::TestMergeOverlappedAttributePath(nlTestSuite * apSuite, void * apContext)
-{
-    TestContext & ctx = *static_cast<TestContext *>(apContext);
-    CHIP_ERROR err    = CHIP_NO_ERROR;
-    err               = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager(), nullptr);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    ClusterInfo clusterInfoList[2];
-
-    clusterInfoList[0].mAttributeId = 1;
-    clusterInfoList[1].mAttributeId = 2;
-
-    {
-        chip::app::ClusterInfo testClusterInfo;
-        testClusterInfo.mAttributeId = 3;
-        NL_TEST_ASSERT(apSuite,
-                       !InteractionModelEngine::GetInstance()->MergeOverlappedAttributePath(clusterInfoList, testClusterInfo));
-    }
-    {
-        chip::app::ClusterInfo testClusterInfo;
-        NL_TEST_ASSERT(apSuite,
-                       InteractionModelEngine::GetInstance()->MergeOverlappedAttributePath(clusterInfoList, testClusterInfo));
-    }
-    {
-        chip::app::ClusterInfo testClusterInfo;
-        testClusterInfo.mAttributeId = 1;
-        testClusterInfo.mListIndex   = 2;
-        NL_TEST_ASSERT(apSuite,
-                       InteractionModelEngine::GetInstance()->MergeOverlappedAttributePath(clusterInfoList, testClusterInfo));
-    }
-}
 } // namespace app
 } // namespace chip
 
@@ -130,7 +98,6 @@ namespace {
 const nlTest sTests[] =
         {
                 NL_TEST_DEF("TestClusterInfoPushRelease", chip::app::TestInteractionModelEngine::TestClusterInfoPushRelease),
-                NL_TEST_DEF("TestMergeOverlappedAttributePath", chip::app::TestInteractionModelEngine::TestMergeOverlappedAttributePath),
                 NL_TEST_SENTINEL()
         };
 // clang-format on

diff --git a/src/app/tests/TestReportingEngine.cpp b/src/app/tests/TestReportingEngine.cpp
@@ -52,6 +52,7 @@ class TestReportingEngine
 {
 public:
     static void TestBuildAndSendSingleReportData(nlTestSuite * apSuite, void * apContext);
+    static void TestMergeOverlappedAttributePath(nlTestSuite * apSuite, void * apContext);
 };
 
 class TestExchangeDelegate : public Messaging::ExchangeDelegate
@@ -107,6 +108,33 @@ void TestReportingEngine::TestBuildAndSendSingleReportData(nlTestSuite * apSuite
     readHandler.Shutdown(app::ReadHandler::ShutdownOptions::AbortCurrentExchange);
 }
 
+void TestReportingEngine::TestMergeOverlappedAttributePath(nlTestSuite * apSuite, void * apContext)
+{
+    TestContext & ctx = *static_cast<TestContext *>(apContext);
+    CHIP_ERROR err    = CHIP_NO_ERROR;
+    err               = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager(), nullptr);
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+    ClusterInfo * clusterInfo = InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.CreateObject();
+    clusterInfo->mAttributeId = 1;
+
+    {
+        chip::app::ClusterInfo testClusterInfo;
+        testClusterInfo.mAttributeId = 3;
+        NL_TEST_ASSERT(apSuite,
+                       !InteractionModelEngine::GetInstance()->GetReportingEngine().MergeOverlappedAttributePath(testClusterInfo));
+    }
+    {
+        chip::app::ClusterInfo testClusterInfo;
+        testClusterInfo.mAttributeId = 1;
+        testClusterInfo.mListIndex   = 2;
+        NL_TEST_ASSERT(apSuite,
+                       InteractionModelEngine::GetInstance()->GetReportingEngine().MergeOverlappedAttributePath(testClusterInfo));
+    }
+
+    InteractionModelEngine::GetInstance()->GetReportingEngine().Shutdown();
+}
+
 } // namespace reporting
 } // namespace app
 } // namespace chip
@@ -116,6 +144,7 @@ namespace {
 const nlTest sTests[] =
 {
     NL_TEST_DEF("CheckBuildAndSendSingleReportData", chip::app::reporting::TestReportingEngine::TestBuildAndSendSingleReportData),
+    NL_TEST_DEF("TestMergeOverlappedAttributePath", chip::app::reporting::TestReportingEngine::TestMergeOverlappedAttributePath),
     NL_TEST_SENTINEL()
 };
 // clang-format on

diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h
@@ -2434,6 +2434,7 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
  *      * #CHIP_IM_MAX_NUM_READ_CLIENT
  *      * #CHIP_IM_MAX_REPORTS_IN_FLIGHT
  *      * #CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS
+ *      * #CHIP_IM_SERVER_MAX_NUM_DIRTY_SET
  *      * #CHIP_IM_MAX_NUM_WRITE_HANDLER
  *      * #CHIP_IM_MAX_NUM_WRITE_CLIENT
  *      * #CHIP_IM_MAX_NUM_TIMED_HANDLER
@@ -2486,6 +2487,15 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
 #define CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS 8
 #endif
 
+/**
+ * @def CHIP_IM_SERVER_MAX_NUM_DIRTY_SET
+ *
+ * @brief Defines the maximum number of dirty set, limits the number of attributes being read or subscribed at the same time.
+ */
+#ifndef CHIP_IM_SERVER_MAX_NUM_DIRTY_SET
+#define CHIP_IM_SERVER_MAX_NUM_DIRTY_SET 8
+#endif
+
 /**
  * @def CHIP_IM_MAX_NUM_WRITE_HANDLER
  *