Skip to content

Commit

Permalink
Un-revert #17417 (#17974)
Browse files Browse the repository at this point in the history 
* [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 <[email protected]>

Co-authored-by: Song GUO <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
@mrjerryjohns @erjiaqing @bzbarsky-apple
3 people authored May 4, 2022
1 parent 3e485a6 commit 9aeecb3
Show file tree
Hide file tree
Showing 5 changed files with 304 additions and 10 deletions.
13 changes: 13 additions & 0 deletions src/app/AttributePathParams.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
{
Expand Down
124 changes: 114 additions & 10 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand Down
30 changes: 30 additions & 0 deletions src/app/reporting/Engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++; }

/**
Expand Down Expand Up @@ -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.
Expand Down
142 changes: 142 additions & 0 deletions src/app/tests/TestReportingEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include <messaging/ExchangeContext.h>
#include <messaging/Flags.h>

#include <cinttypes>
#include <nlunit-test.h>

using TestContext = chip::Test::AppContext;
Expand All @@ -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 <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 %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
Expand Down Expand Up @@ -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<TestContext *>(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
Expand All @@ -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
Expand Down
5 changes: 5 additions & 0 deletions src/lib/support/Pool.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 9aeecb3

Please sign in to comment.