Skip to content

Commit 0b4caea

Browse files
tcarmelveilleuxrestyled-commitsbzbarsky-apple
authored
Introduce AttributeAccessInterface lookup cache (project-chip#32414)
* Introduce AttributeAccessInterface lookup cache - Running TC_DeviceBasicComposition test caused, for a single Wildcard read, 44622 iterations through the AttributeAccessInterface (AAI) lookup loop. This is significant runtime cost on embedded devices, when a wildcard subscription is established or when trying to iterate over changes of attributes. - Instrumented analysis showed that significant locality exists within the lookup, which could be exploited by a cache. - This PR introduces a cache, and its use, in AAI lookups. On same workload as the initial investigation, the total number of iterations of the costly loop went from 44622 --> 4864, a factor of almost 10, for very little additional RAM usage (single entry in cache). Issue project-chip#31405 Testing done: - Added unit tests - Integration tests still pass - TC_BasicDeviceComposition still succeeds * Restyled by clang-format * Appease the Gods of Lint with a BUILD.gn entry * Address review comments by simplifying interface * Restyled by clang-format * Update src/app/AttributeAccessInterfaceCache.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 0590258 commit 0b4caea

5 files changed

+302
-3
lines changed

‎src/app/AttributeAccessInterfaceCache.h

+145
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/*
2+
*
3+
* Copyright (c) 2024 Project CHIP Authors
4+
* All rights reserved.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
#pragma once
19+
20+
#include <stddef.h>
21+
22+
#include <app/AttributeAccessInterface.h>
23+
#include <lib/core/DataModelTypes.h>
24+
25+
namespace chip {
26+
namespace app {
27+
28+
/**
29+
* @brief Cache to make look-up of AttributeAccessInterface (AAI) instances faster.
30+
*
31+
* This cache makes use of the fact that looking-up AttributeAccessInterface
32+
* instances is usually done in loops, during read/subscription wildcard
33+
* expansion, and there is a significant amount of locality.
34+
*
35+
* This cache records both "used" (i.e. uses AAI) and the single last
36+
* "unused" (i.e. does NOT use AAI) entries. Combining positive/negative
37+
* lookup led to factor of ~10 reduction of AAI lookups in total for wildcard
38+
* reads on chip-all-clusters-app, with a cache size of 1. Increasing the size did not
39+
* significantly improve the performance.
40+
*/
41+
class AttributeAccessInterfaceCache
42+
{
43+
public:
44+
enum class CacheResult
45+
{
46+
kCacheMiss,
47+
kDefinitelyUnused,
48+
kDefinitelyUsed
49+
};
50+
51+
AttributeAccessInterfaceCache() { Invalidate(); }
52+
53+
/**
54+
* @brief Invalidate the whole cache. Must be called every time list of AAI registrations changes.
55+
*/
56+
void Invalidate()
57+
{
58+
for (auto & entry : mCacheSlots)
59+
{
60+
entry.Invalidate();
61+
}
62+
mLastUnusedEntry.Invalidate();
63+
}
64+
65+
/**
66+
* @brief Mark that we know a given <`endpointId`, `clusterId`> uses AAI, with instance `attrInterface`
67+
*/
68+
void MarkUsed(EndpointId endpointId, ClusterId clusterId, AttributeAccessInterface * attrInterface)
69+
{
70+
GetCacheSlot(endpointId, clusterId)->Set(endpointId, clusterId, attrInterface);
71+
}
72+
73+
/**
74+
* @brief Mark that we know a given <`endpointId`, `clusterId`> does NOT use AAI.
75+
*/
76+
void MarkUnused(EndpointId endpointId, ClusterId clusterId) { mLastUnusedEntry.Set(endpointId, clusterId, nullptr); }
77+
78+
/**
79+
* @brief Get the AttributeAccessInterface instance for a given <`endpointId`, `clusterId`>, if present in cache.
80+
*
81+
* @param endpointId - Endpoint ID to look-up.
82+
* @param clusterId - Cluster ID to look-up.
83+
* @param outAttributeAccess - If not null, and Get returns `kDefinitelyUsed`, then this is set to the instance pointer.
84+
* @return a for whether the entry is actually used or not.
85+
*/
86+
CacheResult Get(EndpointId endpointId, ClusterId clusterId, AttributeAccessInterface ** outAttributeAccess)
87+
{
88+
if (mLastUnusedEntry.Matches(endpointId, clusterId))
89+
{
90+
return CacheResult::kDefinitelyUnused;
91+
}
92+
93+
AttributeAccessCacheEntry * cacheSlot = GetCacheSlot(endpointId, clusterId);
94+
if (cacheSlot->Matches(endpointId, clusterId) && (cacheSlot->accessor != nullptr))
95+
{
96+
if (outAttributeAccess != nullptr)
97+
{
98+
*outAttributeAccess = cacheSlot->accessor;
99+
}
100+
return CacheResult::kDefinitelyUsed;
101+
}
102+
103+
return CacheResult::kCacheMiss;
104+
}
105+
106+
private:
107+
struct AttributeAccessCacheEntry
108+
{
109+
EndpointId endpointId = kInvalidEndpointId;
110+
ClusterId clusterId = kInvalidClusterId;
111+
AttributeAccessInterface * accessor = nullptr;
112+
113+
void Invalidate()
114+
{
115+
endpointId = kInvalidEndpointId;
116+
clusterId = kInvalidClusterId;
117+
accessor = nullptr;
118+
}
119+
120+
void Set(EndpointId theEndpointId, ClusterId theClusterId, AttributeAccessInterface * theAccessor)
121+
{
122+
endpointId = theEndpointId;
123+
clusterId = theClusterId;
124+
accessor = theAccessor;
125+
}
126+
127+
bool Matches(EndpointId theEndpointId, ClusterId theClusterId) const
128+
{
129+
return (endpointId == theEndpointId) && (clusterId == theClusterId);
130+
}
131+
};
132+
133+
AttributeAccessCacheEntry * GetCacheSlot(EndpointId endpointId, ClusterId clusterId)
134+
{
135+
(void) endpointId;
136+
(void) clusterId;
137+
return &mCacheSlots[0];
138+
}
139+
140+
AttributeAccessCacheEntry mCacheSlots[1];
141+
AttributeAccessCacheEntry mLastUnusedEntry;
142+
};
143+
144+
} // namespace app
145+
} // namespace chip

‎src/app/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ static_library("app") {
250250

251251
sources = [
252252
"AttributeAccessInterface.cpp",
253+
"AttributeAccessInterfaceCache.h",
253254
"AttributePathExpandIterator.cpp",
254255
"AttributePathExpandIterator.h",
255256
"AttributePersistenceProvider.h",

‎src/app/tests/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ chip_test_suite_using_nltest("tests") {
125125

126126
test_sources = [
127127
"TestAclEvent.cpp",
128+
"TestAttributeAccessInterfaceCache.cpp",
128129
"TestAttributePathExpandIterator.cpp",
129130
"TestAttributePersistenceProvider.cpp",
130131
"TestAttributeValueDecoder.cpp",

‎src/app/tests/TestAttributeAccessInterfaceCache.cpp

+129
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/*
2+
*
3+
* Copyright (c) 2024 Project CHIP Authors
4+
* All rights reserved.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
#include <app/AttributeAccessInterface.h>
20+
#include <app/AttributeAccessInterfaceCache.h>
21+
#include <lib/support/UnitTestRegistration.h>
22+
#include <nlunit-test.h>
23+
24+
using namespace chip;
25+
using namespace chip::app;
26+
27+
namespace {
28+
29+
void TestBasicLifecycle(nlTestSuite * inSuite, void * inContext)
30+
{
31+
using CacheResult = AttributeAccessInterfaceCache::CacheResult;
32+
33+
int data1 = 1;
34+
int data2 = 2;
35+
36+
// We alias the pointers to given locations to avoid needing to implement anything
37+
// since the AttributeAccessInterfaceCache only deals in pointers, and never calls
38+
// the API itself.
39+
AttributeAccessInterface * accessor1 = reinterpret_cast<AttributeAccessInterface *>(&data1);
40+
AttributeAccessInterface * accessor2 = reinterpret_cast<AttributeAccessInterface *>(&data2);
41+
42+
AttributeAccessInterfaceCache cache;
43+
44+
// Cache can keep track of at least 1 entry,
45+
AttributeAccessInterface * entry = nullptr;
46+
47+
NL_TEST_ASSERT(inSuite, cache.Get(1, 1, &entry) == CacheResult::kCacheMiss);
48+
NL_TEST_ASSERT(inSuite, entry == nullptr);
49+
cache.MarkUsed(1, 1, accessor1);
50+
51+
NL_TEST_ASSERT(inSuite, cache.Get(1, 1, &entry) == CacheResult::kDefinitelyUsed);
52+
NL_TEST_ASSERT(inSuite, entry == accessor1);
53+
54+
entry = nullptr;
55+
NL_TEST_ASSERT(inSuite, cache.Get(1, 2, &entry) == CacheResult::kCacheMiss);
56+
NL_TEST_ASSERT(inSuite, entry == nullptr);
57+
NL_TEST_ASSERT(inSuite, cache.Get(2, 1, &entry) == CacheResult::kCacheMiss);
58+
NL_TEST_ASSERT(inSuite, entry == nullptr);
59+
60+
cache.MarkUsed(1, 2, accessor1);
61+
62+
entry = nullptr;
63+
NL_TEST_ASSERT(inSuite, cache.Get(1, 2, &entry) == CacheResult::kDefinitelyUsed);
64+
NL_TEST_ASSERT(inSuite, entry == accessor1);
65+
NL_TEST_ASSERT(inSuite, cache.Get(2, 1, &entry) == CacheResult::kCacheMiss);
66+
67+
cache.MarkUsed(1, 2, accessor2);
68+
69+
entry = nullptr;
70+
NL_TEST_ASSERT(inSuite, cache.Get(1, 2, &entry) == CacheResult::kDefinitelyUsed);
71+
NL_TEST_ASSERT(inSuite, entry == accessor2);
72+
// The following should not crash (e.g. output not used if nullptr).
73+
NL_TEST_ASSERT(inSuite, cache.Get(1, 2, nullptr) == CacheResult::kDefinitelyUsed);
74+
75+
// Setting used to nullptr == does not mark used.
76+
cache.MarkUsed(1, 2, nullptr);
77+
entry = nullptr;
78+
NL_TEST_ASSERT(inSuite, cache.Get(1, 2, &entry) == CacheResult::kCacheMiss);
79+
NL_TEST_ASSERT(inSuite, entry == nullptr);
80+
81+
cache.Invalidate();
82+
NL_TEST_ASSERT(inSuite, cache.Get(1, 1, &entry) == CacheResult::kCacheMiss);
83+
NL_TEST_ASSERT(inSuite, entry == nullptr);
84+
NL_TEST_ASSERT(inSuite, cache.Get(1, 2, &entry) == CacheResult::kCacheMiss);
85+
NL_TEST_ASSERT(inSuite, cache.Get(2, 1, &entry) == CacheResult::kCacheMiss);
86+
87+
// Marking unused works, keeps single entry, and is invalidated when invalidated fully.
88+
NL_TEST_ASSERT(inSuite, cache.Get(2, 2, nullptr) != CacheResult::kDefinitelyUnused);
89+
NL_TEST_ASSERT(inSuite, cache.Get(3, 3, nullptr) != CacheResult::kDefinitelyUnused);
90+
cache.MarkUnused(2, 2);
91+
NL_TEST_ASSERT(inSuite, cache.Get(2, 2, nullptr) == CacheResult::kDefinitelyUnused);
92+
NL_TEST_ASSERT(inSuite, cache.Get(3, 3, nullptr) != CacheResult::kDefinitelyUnused);
93+
94+
cache.MarkUnused(3, 3);
95+
NL_TEST_ASSERT(inSuite, cache.Get(2, 2, nullptr) != CacheResult::kDefinitelyUnused);
96+
NL_TEST_ASSERT(inSuite, cache.Get(3, 3, nullptr) == CacheResult::kDefinitelyUnused);
97+
98+
cache.Invalidate();
99+
NL_TEST_ASSERT(inSuite, cache.Get(3, 3, nullptr) != CacheResult::kDefinitelyUnused);
100+
}
101+
102+
// clang-format off
103+
const nlTest sTests[] =
104+
{
105+
NL_TEST_DEF("Basic AttributeAccessInterfaceCache lifecycle works", TestBasicLifecycle),
106+
NL_TEST_SENTINEL()
107+
};
108+
// clang-format on
109+
110+
} // namespace
111+
112+
int TestAttributeAccessInterfaceCache()
113+
{
114+
// clang-format off
115+
nlTestSuite theSuite =
116+
{
117+
"Test for AttributeAccessInterface cache utility",
118+
&sTests[0],
119+
nullptr,
120+
nullptr
121+
};
122+
// clang-format on
123+
124+
nlTestRunner(&theSuite, nullptr);
125+
126+
return (nlTestRunnerStats(&theSuite));
127+
}
128+
129+
CHIP_REGISTER_TEST_SUITE(TestAttributeAccessInterfaceCache)

‎src/app/util/attribute-storage.cpp

+26-3
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#include <app/util/attribute-storage.h>
1919

20+
#include <app/AttributeAccessInterfaceCache.h>
2021
#include <app/AttributePersistenceProvider.h>
2122
#include <app/InteractionModelEngine.h>
2223
#include <app/reporting/reporting.h>
@@ -136,6 +137,7 @@ constexpr const EmberAfDeviceType fixedDeviceTypeList[] = FIXED_DEVI
136137
DataVersion fixedEndpointDataVersions[ZAP_FIXED_ENDPOINT_DATA_VERSION_COUNT];
137138

138139
AttributeAccessInterface * gAttributeAccessOverrides = nullptr;
140+
AttributeAccessInterfaceCache gAttributeAccessInterfaceCache;
139141

140142
// shouldUnregister returns true if the given AttributeAccessInterface should be
141143
// unregistered.
@@ -1364,6 +1366,7 @@ EmberAfGenericClusterFunction emberAfFindClusterFunction(const EmberAfCluster *
13641366

13651367
bool registerAttributeAccessOverride(AttributeAccessInterface * attrOverride)
13661368
{
1369+
gAttributeAccessInterfaceCache.Invalidate();
13671370
for (auto * cur = gAttributeAccessOverrides; cur; cur = cur->GetNext())
13681371
{
13691372
if (cur->Matches(*attrOverride))
@@ -1379,19 +1382,39 @@ bool registerAttributeAccessOverride(AttributeAccessInterface * attrOverride)
13791382

13801383
void unregisterAttributeAccessOverride(AttributeAccessInterface * attrOverride)
13811384
{
1385+
gAttributeAccessInterfaceCache.Invalidate();
13821386
UnregisterMatchingAttributeAccessInterfaces([attrOverride](AttributeAccessInterface * entry) { return entry == attrOverride; });
13831387
}
13841388

13851389
namespace chip {
13861390
namespace app {
1391+
13871392
app::AttributeAccessInterface * GetAttributeAccessOverride(EndpointId endpointId, ClusterId clusterId)
13881393
{
1389-
for (app::AttributeAccessInterface * cur = gAttributeAccessOverrides; cur; cur = cur->GetNext())
1394+
using CacheResult = AttributeAccessInterfaceCache::CacheResult;
1395+
1396+
AttributeAccessInterface * cached = nullptr;
1397+
CacheResult result = gAttributeAccessInterfaceCache.Get(endpointId, clusterId, &cached);
1398+
switch (result)
13901399
{
1391-
if (cur->Matches(endpointId, clusterId))
1400+
case CacheResult::kDefinitelyUnused:
1401+
return nullptr;
1402+
case CacheResult::kDefinitelyUsed:
1403+
return cached;
1404+
case CacheResult::kCacheMiss:
1405+
default:
1406+
// Did not cache yet, search set of AAI registered, and cache if found.
1407+
for (app::AttributeAccessInterface * cur = gAttributeAccessOverrides; cur; cur = cur->GetNext())
13921408
{
1393-
return cur;
1409+
if (cur->Matches(endpointId, clusterId))
1410+
{
1411+
gAttributeAccessInterfaceCache.MarkUsed(endpointId, clusterId, cur);
1412+
return cur;
1413+
}
13941414
}
1415+
1416+
// Did not find AAI registered: mark as definitely not using.
1417+
gAttributeAccessInterfaceCache.MarkUnused(endpointId, clusterId);
13951418
}
13961419

13971420
return nullptr;

0 commit comments

Comments
 (0)