Speed up compilation of all our WriteAttribute machinery. (#11603)

It turns out that instantiating fairly heavy-weight templates hundreds of times is slow to compile. Instead of having an instantiation per attribute, switch to only instantiating the complex templates per _type_ of attribute, with thin per-attribute wrappers for auto-deriving the cluster id and attribute id. This shaves over a minute of wall-clock time off compiling chip-tool for me, and close to 2 minutes of total CPU time.
project-chip · Dec 13, 2021 · 3147633 · 3147633
1 parent 9ecf9e5
commit 3147633
Show file tree

Hide file tree

Showing 25 changed files with 62 additions and 2,876 deletions.
diff --git a/src/app/chip_data_model.gni b/src/app/chip_data_model.gni
@@ -157,7 +157,6 @@ template("chip_data_model") {
         "${invoker.zap_pregenerated_dir}/CHIPClusters.cpp",
         "${invoker.zap_pregenerated_dir}/CHIPClusters.h",
         "${invoker.zap_pregenerated_dir}/CHIPClustersInvoke.cpp",
-        "${invoker.zap_pregenerated_dir}/CHIPClustersWrite.cpp",
         "${invoker.zap_pregenerated_dir}/attribute-size.cpp",
         "${invoker.zap_pregenerated_dir}/callback-stub.cpp",
       ]
@@ -178,7 +177,6 @@ template("chip_data_model") {
         sources += [
           "${invoker.zap_pregenerated_dir}/tests/CHIPClustersTest.cpp",
           "${invoker.zap_pregenerated_dir}/tests/CHIPClustersTest.h",
-          "${invoker.zap_pregenerated_dir}/tests/CHIPClustersTestWrite.cpp",
         ]
       }
     }

diff --git a/src/app/tests/suites/templates/templates.json b/src/app/tests/suites/templates/templates.json
@@ -23,11 +23,6 @@
             "path": "../../../zap-templates/templates/app/tests/CHIPClusters-src.zapt",
             "name": "Tests C++ API",
             "output": "tests/CHIPClustersTest.cpp"
-        },
-        {
-            "path": "../../../zap-templates/templates/app/tests/CHIPClustersWrite-src.zapt",
-            "name": "Tests WriteAttribute instantiations",
-            "output": "tests/CHIPClustersTestWrite.cpp"
         }
     ]
 }
diff --git a/src/app/zap-templates/app-templates.json b/src/app/zap-templates/app-templates.json
@@ -88,11 +88,6 @@
             "path": "templates/app/CHIPClustersInvoke-src.zapt",
             "name": "C++ ZCL Invoke API",
             "output": "CHIPClustersInvoke.cpp"
-        },
-        {
-            "path": "templates/app/CHIPClustersWrite-src.zapt",
-            "name": "C++ ZCL Write API",
-            "output": "CHIPClustersWrite.cpp"
         }
     ]
 }
diff --git a/src/app/zap-templates/templates/app/CHIPClustersWrite-src.zapt b/src/app/zap-templates/templates/app/CHIPClustersWrite-src.zapt
diff --git a/src/app/zap-templates/templates/app/tests/CHIPClustersWrite-src.zapt b/src/app/zap-templates/templates/app/tests/CHIPClustersWrite-src.zapt
diff --git a/src/controller/CHIPCluster.h b/src/controller/CHIPCluster.h
@@ -62,9 +62,45 @@ class DLL_EXPORT ClusterBase
     CHIP_ERROR InvokeCommand(const RequestDataT & requestData, void * context,
                              CommandResponseSuccessCallback<ResponseDataT> successCb, CommandResponseFailureCallback failureCb);
 
+    /**
+     * Functions for writing attributes.  We have lots of different
+     * AttributeInfo but a fairly small set of types that get written.  So we
+     * want to keep the template on AttributeInfo very small, and put all the
+     * work in the template with a small number of instantiations (one per
+     * type).
+     */
+    template <typename AttrType>
+    CHIP_ERROR WriteAttribute(const AttrType & requestData, void * context, ClusterId clusterId, AttributeId attributeId,
+                              WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb)
+    {
+        VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+        auto onSuccessCb = [context, successCb](const app::ConcreteAttributePath & commandPath) {
+            if (successCb != nullptr)
+            {
+                successCb(context);
+            }
+        };
+
+        auto onFailureCb = [context, failureCb](const app::ConcreteAttributePath * commandPath, app::StatusIB status,
+                                                CHIP_ERROR aError) {
+            if (failureCb != nullptr)
+            {
+                failureCb(context, app::ToEmberAfStatus(status.mStatus));
+            }
+        };
+
+        return chip::Controller::WriteAttribute<AttrType>(mDevice->GetSecureSession().Value(), mEndpoint, clusterId, attributeId,
+                                                          requestData, onSuccessCb, onFailureCb);
+    }
+
     template <typename AttributeInfo>
     CHIP_ERROR WriteAttribute(const typename AttributeInfo::Type & requestData, void * context,
-                              WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb);
+                              WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb)
+    {
+        return WriteAttribute(requestData, context, AttributeInfo::GetClusterId(), AttributeInfo::GetAttributeId(), successCb,
+                              failureCb);
+    }
 
 protected:
     ClusterBase(uint16_t cluster) : mClusterId(cluster) {}

diff --git a/src/controller/WriteInteraction.h b/src/controller/WriteInteraction.h
@@ -78,9 +78,15 @@ class WriteCallback final : public app::WriteClient::Callback
     OnDoneCallbackType mOnDone;
 };
 
-template <typename AttributeInfo>
-CHIP_ERROR WriteAttribute(Messaging::ExchangeManager * aExchangeMgr, SessionHandle sessionHandle, chip::EndpointId endpointId,
-                          const typename AttributeInfo::Type & requestCommandData, WriteCallback::OnSuccessCallbackType onSuccessCb,
+/**
+ * Functions for writing attributes.  We have lots of different AttributeInfo
+ * but a fairly small set of types that get written.  So we want to keep the
+ * template on AttributeInfo very small, and put all the work in the template
+ * with a small number of instantiations (one per type).
+ */
+template <typename AttrType>
+CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpointId, ClusterId clusterId, AttributeId attributeId,
+                          const AttrType & requestData, WriteCallback::OnSuccessCallbackType onSuccessCb,
                           WriteCallback::OnErrorCallbackType onErrorCb)
 {
     app::WriteClientHandle handle;
@@ -91,14 +97,22 @@ CHIP_ERROR WriteAttribute(Messaging::ExchangeManager * aExchangeMgr, SessionHand
     VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY);
 
     ReturnErrorOnFailure(app::InteractionModelEngine::GetInstance()->NewWriteClient(handle, callback.get()));
-    ReturnErrorOnFailure(handle.EncodeAttributeWritePayload(
-        chip::app::AttributePathParams(endpointId, AttributeInfo::GetClusterId(), AttributeInfo::GetAttributeId()),
-        requestCommandData));
+    ReturnErrorOnFailure(
+        handle.EncodeAttributeWritePayload(chip::app::AttributePathParams(endpointId, clusterId, attributeId), requestData));
     ReturnErrorOnFailure(handle.SendWriteRequest(sessionHandle));
 
     callback.release();
     return CHIP_NO_ERROR;
 }
 
+template <typename AttributeInfo>
+CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpointId,
+                          const typename AttributeInfo::Type & requestData, WriteCallback::OnSuccessCallbackType onSuccessCb,
+                          WriteCallback::OnErrorCallbackType onErrorCb)
+{
+    return WriteAttribute(sessionHandle, endpointId, AttributeInfo::GetClusterId(), AttributeInfo::GetAttributeId(), requestData,
+                          onSuccessCb, onErrorCb);
+}
+
 } // namespace Controller
 } // namespace chip
diff --git a/src/controller/data_model/BUILD.gn b/src/controller/data_model/BUILD.gn
@@ -22,12 +22,7 @@ chip_data_model("data_model") {
   zap_pregenerated_dir =
       "${chip_root}/zzz_generated/controller-clusters/zap-generated"
 
-  # On Android, we don't really need the tests APIs, and the compiler has
-  # trouble compiling some of those files anyway.
-  #
-  # TODO: This should default to false and consumers should opt in to it
-  # as needed.
-  use_tests_apis = current_os != "android"
+  use_tests_apis = true
   use_default_client_callbacks = true
   allow_circular_includes_from = [ "${chip_root}/src/controller" ]
 }
diff --git a/src/controller/tests/data_model/TestWrite.cpp b/src/controller/tests/data_model/TestWrite.cpp
@@ -148,8 +148,8 @@ void TestWriteInteraction::TestDataResponse(nlTestSuite * apSuite, void * apCont
     auto onFailureCb = [&onFailureCbInvoked](const app::ConcreteAttributePath * attributePath, app::StatusIB status,
                                              CHIP_ERROR aError) { onFailureCbInvoked = true; };
 
-    chip::Controller::WriteAttribute<TestCluster::Attributes::ListStructOctetString::TypeInfo>(
-        &ctx.GetExchangeManager(), sessionHandle, kTestEndpointId, value, onSuccessCb, onFailureCb);
+    chip::Controller::WriteAttribute<TestCluster::Attributes::ListStructOctetString::TypeInfo>(sessionHandle, kTestEndpointId,
+                                                                                               value, onSuccessCb, onFailureCb);
 
     ctx.DrainAndServiceIO();
 
@@ -189,8 +189,8 @@ void TestWriteInteraction::TestAttributeError(nlTestSuite * apSuite, void * apCo
         onFailureCbInvoked = true;
     };
 
-    chip::Controller::WriteAttribute<TestCluster::Attributes::ListStructOctetString::TypeInfo>(
-        &ctx.GetExchangeManager(), sessionHandle, kTestEndpointId, value, onSuccessCb, onFailureCb);
+    chip::Controller::WriteAttribute<TestCluster::Attributes::ListStructOctetString::TypeInfo>(sessionHandle, kTestEndpointId,
+                                                                                               value, onSuccessCb, onFailureCb);
 
     ctx.DrainAndServiceIO();
 

diff --git a/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj b/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj
@@ -55,7 +55,6 @@
 		2F79A67726CE6672006377B0 /* im-client-callbacks.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2F79A67626CE6672006377B0 /* im-client-callbacks.cpp */; };
 		2FD775552695557E00FF4B12 /* error-mapping.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2FD775542695557E00FF4B12 /* error-mapping.cpp */; };
 		5129BCFD26A9EE3300122DDF /* CHIPError.h in Headers */ = {isa = PBXBuildFile; fileRef = 5129BCFC26A9EE3300122DDF /* CHIPError.h */; settings = {ATTRIBUTES = (Public, ); }; };
-		515CCFD227351183002A3C82 /* CHIPClustersTestWrite.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 515CCFD127351183002A3C82 /* CHIPClustersTestWrite.cpp */; };
 		991DC0842475F45400C13860 /* CHIPDeviceController.h in Headers */ = {isa = PBXBuildFile; fileRef = 991DC0822475F45400C13860 /* CHIPDeviceController.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		991DC0892475F47D00C13860 /* CHIPDeviceController.mm in Sources */ = {isa = PBXBuildFile; fileRef = 991DC0872475F47D00C13860 /* CHIPDeviceController.mm */; };
 		991DC08B247704DC00C13860 /* CHIPLogging.h in Headers */ = {isa = PBXBuildFile; fileRef = 991DC08A247704DC00C13860 /* CHIPLogging.h */; };
@@ -142,7 +141,6 @@
 		2F79A67626CE6672006377B0 /* im-client-callbacks.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = "im-client-callbacks.cpp"; path = "../../../app/util/im-client-callbacks.cpp"; sourceTree = "<group>"; };
 		2FD775542695557E00FF4B12 /* error-mapping.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = "error-mapping.cpp"; path = "../../../app/util/error-mapping.cpp"; sourceTree = "<group>"; };
 		5129BCFC26A9EE3300122DDF /* CHIPError.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = CHIPError.h; path = CHIP/CHIPError.h; sourceTree = "<group>"; };
-		515CCFD127351183002A3C82 /* CHIPClustersTestWrite.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = CHIPClustersTestWrite.cpp; path = "../../../zzz_generated/controller-clusters/zap-generated/tests/CHIPClustersTestWrite.cpp"; sourceTree = "<group>"; };
 		991DC0822475F45400C13860 /* CHIPDeviceController.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CHIPDeviceController.h; sourceTree = "<group>"; };
 		991DC0872475F47D00C13860 /* CHIPDeviceController.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = CHIPDeviceController.mm; sourceTree = "<group>"; };
 		991DC08A247704DC00C13860 /* CHIPLogging.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CHIPLogging.h; sourceTree = "<group>"; };
@@ -242,7 +240,6 @@
 		B20252832459E34F00F97062 = {
 			isa = PBXGroup;
 			children = (
-				515CCFD127351183002A3C82 /* CHIPClustersTestWrite.cpp */,
 				5129BCFC26A9EE3300122DDF /* CHIPError.h */,
 				BA107AEE2470CFBB004287EB /* chip_xcode_build_connector.sh */,
 				B202528F2459E34F00F97062 /* CHIP */,
@@ -481,7 +478,6 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
-				515CCFD227351183002A3C82 /* CHIPClustersTestWrite.cpp in Sources */,
 				2C8C8FC2253E0C2100797F05 /* CHIPPersistentStorageDelegateBridge.mm in Sources */,
 				2CB7163C252E8A7C0026E2BB /* CHIPDevicePairingDelegateBridge.mm in Sources */,
 				997DED162695343400975E97 /* CHIPThreadOperationalDataset.mm in Sources */,

diff --git a/zzz_generated/all-clusters-app/zap-generated/CHIPClustersWrite.cpp b/zzz_generated/all-clusters-app/zap-generated/CHIPClustersWrite.cpp
diff --git a/zzz_generated/bridge-app/zap-generated/CHIPClustersWrite.cpp b/zzz_generated/bridge-app/zap-generated/CHIPClustersWrite.cpp