Introduce AttributeAccessInterface lookup cache

src/app/AttributeAccessInterfaceCache.h

@@ -0,0 +1,139 @@
+/*
+ *
+ *    Copyright (c) 2024 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+#pragma once
+
+#include <stddef.h>
+
+#include <app/AttributeAccessInterface.h>
+#include <lib/core/DataModelTypes.h>
+
+namespace chip {
+namespace app {
+
+/**
+ * @brief Cache to make look-up of AttributeAccessInterface (AAI) instances faster.
+ *
+ * @tparam N - The size of the cache (current suggested value == 1)
+ *
+ * This cache makes use of the fact that looking-up AttributeAccessInterface
+ * instances is usually done in loops, during read/subscription wildcard
+ * expansion, and there is a significant amount of locality.
+ *
+ * This cache records both "used" (i.e. uses AAI) and the single last
+ * "unused" (i.e. does NOT use AAI) entries. Combining positive/negative
+ * lookup led to factor of ~10 reduction of AAI lookups in total for wildcard
+ * reads on chip-all-clusters-app, with a cache size of 1. The size did not
+ * significantly improve the performance, but `N` is left to support better
+ * hashing/storage algorithms in the future if needed.
+ */
+template <size_t N>
+class AttributeAccessInterfaceCache
+{
+public:
+    AttributeAccessInterfaceCache() { Invalidate(); }
+
+    /**
+     * @brief Invalidate the whole cache. Must be called every time list of AAI registrations changes.
+     */
+    void Invalidate()
+    {
+        for (auto & entry : mCacheSlots)
+        {
+            entry.Invalidate();
+        }
+        mLastUnusedEntry.Invalidate();
+    }
+
+    /**
+     * @brief Mark that we know a given <`endpointId`, `clusterId`> uses AAI, with instance `attrInterface`
+     */
+    void MarkUsed(EndpointId endpointId, ClusterId clusterId, AttributeAccessInterface * attrInterface)
+    {
+        GetCacheSlot(endpointId, clusterId)->Set(endpointId, clusterId, attrInterface);
+    }
+
+    /**
+     * @brief Mark that we know a given <`endpointId`, `clusterId`> does NOT use AAI.
+     */
+    void MarkUnused(EndpointId endpointId, ClusterId clusterId) { mLastUnusedEntry.Set(endpointId, clusterId, nullptr); }
+
+    /**
+     * @brief Get the AttributeAccessInterface instance for a given <`endpointId`, `clusterId`>, if present in cache.
+     *
+     * @param endpointId - Endpoint ID to look-up.
+     * @param clusterId - Cluster ID to look-up.
+     * @return the instance pointer on cache hit, or nullptr on cache miss.
+     */
+    AttributeAccessInterface * Get(EndpointId endpointId, ClusterId clusterId)
+    {
+        AttributeAccessCacheEntry * cacheSlot = GetCacheSlot(endpointId, clusterId);
+        if (cacheSlot->Matches(endpointId, clusterId) && (cacheSlot->accessor != nullptr))
+        {
+            return cacheSlot->accessor;
+        }
+
+        return nullptr;
+    }
+
+    /**
+     * @brief returns true if <`endpointId`, `clusterId`> was last marked as NOT to using AAI.
+     *
+     * May return false even though it doesn't use AAI, on cache miss of unused slot.
+     */
+    bool IsUnused(EndpointId endpointId, ClusterId clusterId) const { return mLastUnusedEntry.Matches(endpointId, clusterId); }
+
+private:
+    struct AttributeAccessCacheEntry
+    {
+        EndpointId endpointId               = kInvalidEndpointId;
+        ClusterId clusterId                 = kInvalidClusterId;
+        AttributeAccessInterface * accessor = nullptr;
+
+        void Invalidate()
+        {
+            endpointId = kInvalidEndpointId;
+            clusterId  = kInvalidClusterId;
+            accessor   = nullptr;
+        }
+
+        void Set(EndpointId theEndpointId, ClusterId theClusterId, AttributeAccessInterface * theAccessor)
+        {
+            endpointId = theEndpointId;
+            clusterId  = theClusterId;
+            accessor   = theAccessor;
+        }
+
+        bool Matches(EndpointId theEndpointId, ClusterId theClusterId) const
+        {
+            return (endpointId == theEndpointId) && (clusterId == theClusterId);
+        }
+    };
+
+    AttributeAccessCacheEntry * GetCacheSlot(EndpointId endpointId, ClusterId clusterId)
+    {
+        // A future implementation may use a better method.
+        size_t slotId = (clusterId ^ endpointId) & (N - 1);
+        return &mCacheSlots[slotId];
+    }
+
+    AttributeAccessCacheEntry mCacheSlots[N];
+    AttributeAccessCacheEntry mLastUnusedEntry;
+};
+
+} // namespace app
+} // namespace chip

src/app/BUILD.gn

@@ -250,6 +250,7 @@ static_library("app") {
 
   sources = [
     "AttributeAccessInterface.cpp",
+    "AttributeAccessInterfaceCache.h",
     "AttributePathExpandIterator.cpp",
     "AttributePathExpandIterator.h",
     "AttributePersistenceProvider.h",

src/app/tests/BUILD.gn

@@ -125,6 +125,7 @@ chip_test_suite_using_nltest("tests") {
 
   test_sources = [
     "TestAclEvent.cpp",
+    "TestAttributeAccessInterfaceCache.cpp",
     "TestAttributePathExpandIterator.cpp",
     "TestAttributePersistenceProvider.cpp",
     "TestAttributeValueDecoder.cpp",

src/app/tests/TestAttributeAccessInterfaceCache.cpp

@@ -0,0 +1,105 @@
+/*
+ *
+ *    Copyright (c) 2024 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <app/AttributeAccessInterface.h>
+#include <app/AttributeAccessInterfaceCache.h>
+#include <lib/support/UnitTestRegistration.h>
+#include <nlunit-test.h>
+
+using namespace chip;
+using namespace chip::app;
+
+namespace {
+
+void TestBasicLifecycle(nlTestSuite * inSuite, void * inContext)
+{
+    int data1 = 1;
+    int data2 = 2;
+
+    // We alias the pointers to given locations to avoid needing to implement anything
+    // since the AttributeAccessInterfaceCache only deals in pointers, and never calls
+    // the API itself.
+    AttributeAccessInterface * accessor1 = reinterpret_cast<AttributeAccessInterface *>(&data1);
+    AttributeAccessInterface * accessor2 = reinterpret_cast<AttributeAccessInterface *>(&data2);
+
+    AttributeAccessInterfaceCache<1> cache;
+
+    // Cache can keep track of at least 1 entry,
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 1) == nullptr);
+    NL_TEST_ASSERT(inSuite, !cache.IsUnused(1, 1));
+    cache.MarkUsed(1, 1, accessor1);
+
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 1) == accessor1);
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 2) == nullptr);
+    NL_TEST_ASSERT(inSuite, cache.Get(2, 1) == nullptr);
+
+    cache.MarkUsed(1, 2, accessor1);
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 2) == accessor1);
+    NL_TEST_ASSERT(inSuite, cache.Get(2, 1) == nullptr);
+
+    cache.MarkUsed(1, 2, accessor2);
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 2) == accessor2);
+
+    cache.Invalidate();
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 1) == nullptr);
+    NL_TEST_ASSERT(inSuite, cache.Get(1, 2) == nullptr);
+    NL_TEST_ASSERT(inSuite, cache.Get(2, 1) == nullptr);
+
+    // Marking unused works, keeps single entry, and is invalidated when invalidated fully.
+    NL_TEST_ASSERT(inSuite, !cache.IsUnused(2, 2));
+    NL_TEST_ASSERT(inSuite, !cache.IsUnused(3, 3));
+    cache.MarkUnused(2, 2);
+    NL_TEST_ASSERT(inSuite, cache.IsUnused(2, 2));
+    NL_TEST_ASSERT(inSuite, !cache.IsUnused(3, 3));
+
+    cache.MarkUnused(3, 3);
+    NL_TEST_ASSERT(inSuite, !cache.IsUnused(2, 2));
+    NL_TEST_ASSERT(inSuite, cache.IsUnused(3, 3));
+
+    cache.Invalidate();
+    NL_TEST_ASSERT(inSuite, !cache.IsUnused(3, 3));
+}
+
+// clang-format off
+const nlTest sTests[] =
+{
+    NL_TEST_DEF("Basic AttributeAccessInterfaceCache lifecycle works", TestBasicLifecycle),
+    NL_TEST_SENTINEL()
+};
+// clang-format on
+
+} // namespace
+
+int TestAttributeAccessInterfaceCache()
+{
+    // clang-format off
+    nlTestSuite theSuite =
+    {
+        "Test for AttributeAccessInterface cache utility",
+        &sTests[0],
+        nullptr,
+        nullptr
+    };
+    // clang-format on
+
+    nlTestRunner(&theSuite, nullptr);
+
+    return (nlTestRunnerStats(&theSuite));
+}
+
+CHIP_REGISTER_TEST_SUITE(TestAttributeAccessInterfaceCache)

src/app/util/attribute-storage.cpp

@@ -17,6 +17,7 @@
 
 #include <app/util/attribute-storage.h>
 
+#include <app/AttributeAccessInterfaceCache.h>
 #include <app/AttributePersistenceProvider.h>
 #include <app/InteractionModelEngine.h>
 #include <app/reporting/reporting.h>
@@ -136,6 +137,7 @@ constexpr const EmberAfDeviceType fixedDeviceTypeList[]             = FIXED_DEVI
 DataVersion fixedEndpointDataVersions[ZAP_FIXED_ENDPOINT_DATA_VERSION_COUNT];
 
 AttributeAccessInterface * gAttributeAccessOverrides = nullptr;
+AttributeAccessInterfaceCache<1> gAttributeAccessInterfaceCache;
 
 // shouldUnregister returns true if the given AttributeAccessInterface should be
 // unregistered.
@@ -1364,6 +1366,7 @@ EmberAfGenericClusterFunction emberAfFindClusterFunction(const EmberAfCluster *
 
 bool registerAttributeAccessOverride(AttributeAccessInterface * attrOverride)
 {
+    gAttributeAccessInterfaceCache.Invalidate();
     for (auto * cur = gAttributeAccessOverrides; cur; cur = cur->GetNext())
     {
         if (cur->Matches(*attrOverride))
@@ -1379,21 +1382,39 @@ bool registerAttributeAccessOverride(AttributeAccessInterface * attrOverride)
 
 void unregisterAttributeAccessOverride(AttributeAccessInterface * attrOverride)
 {
+    gAttributeAccessInterfaceCache.Invalidate();
     UnregisterMatchingAttributeAccessInterfaces([attrOverride](AttributeAccessInterface * entry) { return entry == attrOverride; });
 }
 
 namespace chip {
 namespace app {
 app::AttributeAccessInterface * GetAttributeAccessOverride(EndpointId endpointId, ClusterId clusterId)
 {
+    // If endpoint/cluster is definitely not for AttributeAccessInterface, immediately skip all searches.
+    if (gAttributeAccessInterfaceCache.IsUnused(endpointId, clusterId))
+    {
+        return nullptr;
+    }
+
+    // Find in cache, hope for a match.
+    app::AttributeAccessInterface * cached = gAttributeAccessInterfaceCache.Get(endpointId, clusterId);
+    if (cached != nullptr)
+    {
+        return cached;
+    }
+
+    // Did not cache yet, search set of AAI registered, and cache if found.
     for (app::AttributeAccessInterface * cur = gAttributeAccessOverrides; cur; cur = cur->GetNext())
     {
         if (cur->Matches(endpointId, clusterId))
         {
+            gAttributeAccessInterfaceCache.MarkUsed(endpointId, clusterId, cur);
             return cur;
         }
     }
 
+    // Did not failing AAI registered: mark as definitely not using.
+    gAttributeAccessInterfaceCache.MarkUnused(endpointId, clusterId);
     return nullptr;
 }