From 11630569f7020fc9f362152a2478c086790e3f9f Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 30 Jan 2024 12:43:48 -0500 Subject: [PATCH] Reduce CommandHandler::TryAddResponseData compiled code size (#31631) --------- Co-authored-by: Boris Zbarsky --- src/app/CommandHandler.h | 74 ++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 659828cc7d9eb8..b90cb821244336 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -360,24 +360,7 @@ class CommandHandler template CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) { - // This method, templated with CommandData, captures all the components needs - // from CommandData with as little code as possible. This in theory should - // reduce compiled code size. - // - // TODO(#30453): Verify the accuracy of the theory outlined below. - // - // Theory on code reduction: Previously, non-essential code was unnecessarily - // templated, leading to compilation and duplication N times. The lambda - // function below mitigates this issue by isolating only the code segments - // that genuinely require templating, thereby minimizing duplicate compiled - // code. - ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, - CommandData::GetCommandId() }; - auto encodeCommandDataClosure = [&](TLV::TLVWriter & writer) -> CHIP_ERROR { - return DataModel::Encode(writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData); - }; - return TryAddingResponse( - [&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, responsePath, encodeCommandDataClosure); }); + return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, aData); }); } /** @@ -567,18 +550,21 @@ class CommandHandler CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus); /** - * If this function fails, it may leave our TLV buffer in an inconsistent state. Callers should snapshot as needed before - * calling this function, and roll back as needed afterward. + * Non-templated function called before DataModel::Encode when attempting to add a response, + * which does all the work needed before encoding the actual type-dependent data into the buffer. * - * @param [in] aRequestCommandPath the concrete path of the command we are - * responding to. - * @param [in] aResponseCommandPath the concrete command response path. - * @param [in] encodeCommandDataFunction A lambda function responsible for - * encoding the CommandData field. + * **Important:** If this function fails, the TLV buffer may be left in an inconsistent state. + * Callers should create snapshots as necessary before invoking this function and implement + * rollback mechanisms if needed. + * + * **Usage:** This function is intended to be called exclusively by TryAddResponseData. It was + * factored out to optimize code size. + * + * @param aRequestCommandPath The concrete path of the command being responded to. + * @param aResponseCommandPath The concrete path of the command response. */ - template - CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aResponseCommandPath, - Function && encodeCommandDataFunction) + CHIP_ERROR TryAddResponseDataPreEncode(const ConcreteCommandPath & aRequestCommandPath, + const ConcreteCommandPath & aResponseCommandPath) { // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); @@ -587,11 +573,39 @@ class CommandHandler prepareParams.SetStartOrEndDataStruct(false); ScopedChange internalCallToAddResponse(mInternalCallToAddResponseData, true); - ReturnErrorOnFailure(PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams)); + return PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams); + } + + // TODO(#31627): It would be awesome if we could remove this template all together. + /** + * If this function fails, it may leave our TLV buffer in an inconsistent state. + * Callers should snapshot as needed before calling this function, and roll back + * as needed afterward. + * + * @param [in] aRequestCommandPath the concrete path of the command we are + * responding to. + * @param [in] aData the data for the response. + */ + template + CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) + { + // This method, templated with CommandData, captures all the components needs + // from CommandData with as little code as possible. + // + // Previously, non-essential code was unnecessarily templated, leading to + // compilation and duplication N times. By isolating only the code segments + // that genuinely require templating, minimizes duplicate compiled code. + ConcreteCommandPath responseCommandPath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, + CommandData::GetCommandId() }; + ReturnErrorOnFailure(TryAddResponseDataPreEncode(aRequestCommandPath, responseCommandPath)); TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(encodeCommandDataFunction(*writer)); + ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData)); + // FinishCommand technically should be refactored out as it is not a command that needs templating. + // But, because there is only a single function call, keeping it here takes less code. If there is + // ever more code between DataModel::Encode and the end of this function, it should be broken out into + // TryAddResponseDataPostEncode. return FinishCommand(/* aEndDataStruct = */ false); }