Skip to content

Commit

Permalink
Only Cache the path with wildcard attribute (#17887)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed Sep 29, 2023
1 parent c8512d1 commit 4400848
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 16 deletions.
4 changes: 2 additions & 2 deletions src/app/AttributePathExpandIterator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,12 @@ bool AttributePathExpandIterator::Next()
{
for (; mpAttributePath != nullptr; (mpAttributePath = mpAttributePath->mpNext, mEndpointIndex = UINT16_MAX))
{
mOutputPath.mExpanded = mpAttributePath->mValue.HasAttributeWildcard();
mOutputPath.mExpanded = mpAttributePath->mValue.IsWildcardPath();

if (mEndpointIndex == UINT16_MAX)
{
// Special case: If this is a concrete path, we just return its value as-is.
if (!mpAttributePath->mValue.HasAttributeWildcard())
if (!mpAttributePath->mValue.IsWildcardPath())
{
mOutputPath.mEndpointId = mpAttributePath->mValue.mEndpointId;
mOutputPath.mClusterId = mpAttributePath->mValue.mClusterId;
Expand Down
2 changes: 1 addition & 1 deletion src/app/AttributePathParams.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ struct AttributePathParams

AttributePathParams() {}

bool HasAttributeWildcard() const { return HasWildcardEndpointId() || HasWildcardClusterId() || HasWildcardAttributeId(); }
bool IsWildcardPath() const { return HasWildcardEndpointId() || HasWildcardClusterId() || HasWildcardAttributeId(); }

/**
* SPEC 8.9.2.2
Expand Down
2 changes: 1 addition & 1 deletion src/app/ClusterStateCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ CHIP_ERROR ClusterStateCache::OnUpdateDataVersionFilterList(DataVersionFilterIBs

for (auto & attribute : aAttributePaths)
{
if (attribute.HasAttributeWildcard())
if (attribute.HasWildcardAttributeId())
{
mRequestPathSet.insert(attribute);
}
Expand Down
4 changes: 2 additions & 2 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(ObjectList<Att
{
bool duplicate = false;
// skip all wildcard paths and invalid concrete attribute
if (path1->mValue.HasAttributeWildcard() ||
if (path1->mValue.IsWildcardPath() ||
!emberAfContainsAttribute(path1->mValue.mEndpointId, path1->mValue.mClusterId, path1->mValue.mAttributeId, true))
{
prev = path1;
Expand All @@ -881,7 +881,7 @@ void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(ObjectList<Att
continue;
}

if (path2->mValue.HasAttributeWildcard() && path2->mValue.IsAttributePathSupersetOf(path1->mValue))
if (path2->mValue.IsWildcardPath() && path2->mValue.IsAttributePathSupersetOf(path1->mValue))
{
duplicate = true;
break;
Expand Down
59 changes: 49 additions & 10 deletions src/controller/tests/data_model/TestRead.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
app::InteractionModelEngine::GetInstance()->RegisterReadHandlerAppCallback(&gTestReadInteraction);

int testId = 0;
// Initial Read of E2C3A1, E2C3A2 and E3C2A2.
// Read of E2C3A1, E2C3A2 and E3C2A2.
// Expect no versions would be cached.
{
testId++;
Expand Down Expand Up @@ -419,7 +419,46 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
NL_TEST_ASSERT(apSuite, !version2.HasValue());
delegate.mNumAttributeResponse = 0;
}
// Second read of E2C2A* and E3C2A2. We cannot use the stored data versions in the cache since there is no cached version from

// Read of E*C2A2, E2C2A2 and E3C2A2 where 2nd, 3rd concrete paths are part of first wildcard path, would be deduplicate,
// E*C2A2 don't have wildcard attribute so no version would be cached.
// Expect no versions would be cached.
{
testId++;
ChipLogProgress(DataManagement, "\t -- Running Read with ClusterStateCache Test ID %d", testId);
app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(),
cache.GetBufferedCallback(), chip::app::ReadClient::InteractionType::Read);
chip::app::AttributePathParams attributePathParams1[3];
attributePathParams1[0].mEndpointId = kInvalidEndpointId;
attributePathParams1[0].mClusterId = Test::MockClusterId(2);
attributePathParams1[0].mAttributeId = Test::MockAttributeId(2);

attributePathParams1[1].mEndpointId = Test::kMockEndpoint2;
attributePathParams1[1].mClusterId = Test::MockClusterId(2);
attributePathParams1[1].mAttributeId = Test::MockAttributeId(2);

attributePathParams1[2].mEndpointId = Test::kMockEndpoint3;
attributePathParams1[2].mClusterId = Test::MockClusterId(2);
attributePathParams1[2].mAttributeId = Test::MockAttributeId(2);

readPrepareParams.mpAttributePathParamsList = attributePathParams1;
readPrepareParams.mAttributePathParamsListSize = 3;
err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2);
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
Optional<DataVersion> version1;
NL_TEST_ASSERT(apSuite, cache.GetVersion(Test::kMockEndpoint2, Test::MockClusterId(3), version1) == CHIP_NO_ERROR);
NL_TEST_ASSERT(apSuite, !version1.HasValue());
Optional<DataVersion> version2;
NL_TEST_ASSERT(apSuite, cache.GetVersion(Test::kMockEndpoint3, Test::MockClusterId(2), version2) == CHIP_NO_ERROR);
NL_TEST_ASSERT(apSuite, !version2.HasValue());
delegate.mNumAttributeResponse = 0;
}

// read of E2C2A* and E3C2A2. We cannot use the stored data versions in the cache since there is no cached version from
// previous test. Expect cache E2C2 version
{
testId++;
Expand Down Expand Up @@ -452,7 +491,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
delegate.mNumAttributeResponse = 0;
}

// Third read of E2C3A1, E2C3A2, and E3C2A2. It would use the stored data versions in the cache since our subsequent read's C1A1
// Read of E2C3A1, E2C3A2, and E3C2A2. It would use the stored data versions in the cache since our subsequent read's C1A1
// path intersects with previous cached data version Expect no E2C3 attributes in report, only E3C2A1 attribute in report
{
testId++;
Expand Down Expand Up @@ -489,7 +528,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
delegate.mNumAttributeResponse = 0;
}

// Fourth read of E2C3A* and E3C2A2. It would use the stored data versions in the cache since our subsequent read's C1A* path
// Read of E2C3A* and E3C2A2. It would use the stored data versions in the cache since our subsequent read's C1A* path
// intersects with previous cached data version Expect no C1 attributes in report, only E3C2A2 attribute in report
{
testId++;
Expand Down Expand Up @@ -523,7 +562,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit

Test::BumpVersion();

// Fifth read of E2C3A1, E2C3A2 and E3C2A2. It would use the stored data versions in the cache since our subsequent read's C1A*
// Read of E2C3A1, E2C3A2 and E3C2A2. It would use the stored data versions in the cache since our subsequent read's C1A*
// path intersects with previous cached data version, server's version is changed. Expect E2C3A1, E2C3A2 and E3C2A2 attribute in
// report, and invalidate the cached pending and committed data version since no wildcard attributes exists in mRequestPathSet.
{
Expand Down Expand Up @@ -561,7 +600,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
delegate.mNumAttributeResponse = 0;
}

// Sixth read of E2C3A1, E2C3A2 and E3C2A2. It would use none stored data versions in the cache since previous read does not
// Read of E2C3A1, E2C3A2 and E3C2A2. It would use none stored data versions in the cache since previous read does not
// cache any committed data version. Expect E2C3A1, E2C3A2 and E3C2A2 attribute in report
{
testId++;
Expand Down Expand Up @@ -598,7 +637,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
delegate.mNumAttributeResponse = 0;
}

// Seventh read of E2C3A* and E3C2A2, here there is no cached data version filter
// Read of E2C3A* and E3C2A2, here there is no cached data version filter
// Expect E2C3A* attributes in report, and E3C2A2 attribute in report and cache latest data version
{
testId++;
Expand Down Expand Up @@ -630,7 +669,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
delegate.mNumAttributeResponse = 0;
}

// Eighth read of E2C3A* and E3C2A2, and inject a large amount of event path list, then it would try to apply previous cache
// Read of E2C3A* and E3C2A2, and inject a large amount of event path list, then it would try to apply previous cache
// latest data version and construct data version list but no enough memory, finally fully rollback data version filter. Expect
// E2C3A* attributes in report, and E3C2A2 attribute in report
{
Expand Down Expand Up @@ -671,7 +710,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit

Test::BumpVersion();

// Ninth read of E1C2A* and E2C3A* and E2C2A*, it would use C1 cached version to construct DataVersionFilter, but version has
// Read of E1C2A* and E2C3A* and E2C2A*, it would use C1 cached version to construct DataVersionFilter, but version has
// changed in server. Expect E1C2A* and C2C3A* and E2C2A* attributes in report, and cache their versions
{
testId++;
Expand Down Expand Up @@ -713,7 +752,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
delegate.mNumAttributeResponse = 0;
}

// Tenth read of E1C2A*(3 attributes) and E2C3A*(5 attributes) and E2C2A*(4 attributes), and inject a large amount of event path
// Read of E1C2A*(3 attributes) and E2C3A*(5 attributes) and E2C2A*(4 attributes), and inject a large amount of event path
// list, then it would try to apply previous cache latest data version and construct data version list with the ordering from
// largest cluster size to smallest cluster size(C2, C3, C1) but no enough memory, finally partially rollback data version
// filter with only C2. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
Expand Down

0 comments on commit 4400848

Please sign in to comment.