Skip to content

Commit 30a7636

Browse files
Reduce CommandHandler::TryAddResponseData compiled code size (project-chip#31631)
--------- Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 48e7bae commit 30a7636

File tree

1 file changed

+44
-30
lines changed

1 file changed

+44
-30
lines changed

src/app/CommandHandler.h

+44-30
Original file line numberDiff line numberDiff line change
@@ -360,24 +360,7 @@ class CommandHandler
360360
template <typename CommandData>
361361
CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
362362
{
363-
// This method, templated with CommandData, captures all the components needs
364-
// from CommandData with as little code as possible. This in theory should
365-
// reduce compiled code size.
366-
//
367-
// TODO(#30453): Verify the accuracy of the theory outlined below.
368-
//
369-
// Theory on code reduction: Previously, non-essential code was unnecessarily
370-
// templated, leading to compilation and duplication N times. The lambda
371-
// function below mitigates this issue by isolating only the code segments
372-
// that genuinely require templating, thereby minimizing duplicate compiled
373-
// code.
374-
ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId,
375-
CommandData::GetCommandId() };
376-
auto encodeCommandDataClosure = [&](TLV::TLVWriter & writer) -> CHIP_ERROR {
377-
return DataModel::Encode(writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData);
378-
};
379-
return TryAddingResponse(
380-
[&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, responsePath, encodeCommandDataClosure); });
363+
return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, aData); });
381364
}
382365

383366
/**
@@ -567,18 +550,21 @@ class CommandHandler
567550
CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus);
568551

569552
/**
570-
* If this function fails, it may leave our TLV buffer in an inconsistent state. Callers should snapshot as needed before
571-
* calling this function, and roll back as needed afterward.
553+
* Non-templated function called before DataModel::Encode when attempting to add a response,
554+
* which does all the work needed before encoding the actual type-dependent data into the buffer.
572555
*
573-
* @param [in] aRequestCommandPath the concrete path of the command we are
574-
* responding to.
575-
* @param [in] aResponseCommandPath the concrete command response path.
576-
* @param [in] encodeCommandDataFunction A lambda function responsible for
577-
* encoding the CommandData field.
556+
* **Important:** If this function fails, the TLV buffer may be left in an inconsistent state.
557+
* Callers should create snapshots as necessary before invoking this function and implement
558+
* rollback mechanisms if needed.
559+
*
560+
* **Usage:** This function is intended to be called exclusively by TryAddResponseData. It was
561+
* factored out to optimize code size.
562+
*
563+
* @param aRequestCommandPath The concrete path of the command being responded to.
564+
* @param aResponseCommandPath The concrete path of the command response.
578565
*/
579-
template <typename Function>
580-
CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aResponseCommandPath,
581-
Function && encodeCommandDataFunction)
566+
CHIP_ERROR TryAddResponseDataPreEncode(const ConcreteCommandPath & aRequestCommandPath,
567+
const ConcreteCommandPath & aResponseCommandPath)
582568
{
583569
// Return early in case of requests targeted to a group, since they should not add a response.
584570
VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR);
@@ -587,11 +573,39 @@ class CommandHandler
587573
prepareParams.SetStartOrEndDataStruct(false);
588574

589575
ScopedChange<bool> internalCallToAddResponse(mInternalCallToAddResponseData, true);
590-
ReturnErrorOnFailure(PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams));
576+
return PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams);
577+
}
578+
579+
// TODO(#31627): It would be awesome if we could remove this template all together.
580+
/**
581+
* If this function fails, it may leave our TLV buffer in an inconsistent state.
582+
* Callers should snapshot as needed before calling this function, and roll back
583+
* as needed afterward.
584+
*
585+
* @param [in] aRequestCommandPath the concrete path of the command we are
586+
* responding to.
587+
* @param [in] aData the data for the response.
588+
*/
589+
template <typename CommandData>
590+
CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
591+
{
592+
// This method, templated with CommandData, captures all the components needs
593+
// from CommandData with as little code as possible.
594+
//
595+
// Previously, non-essential code was unnecessarily templated, leading to
596+
// compilation and duplication N times. By isolating only the code segments
597+
// that genuinely require templating, minimizes duplicate compiled code.
598+
ConcreteCommandPath responseCommandPath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId,
599+
CommandData::GetCommandId() };
600+
ReturnErrorOnFailure(TryAddResponseDataPreEncode(aRequestCommandPath, responseCommandPath));
591601
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
592602
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
593-
ReturnErrorOnFailure(encodeCommandDataFunction(*writer));
603+
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData));
594604

605+
// FinishCommand technically should be refactored out as it is not a command that needs templating.
606+
// But, because there is only a single function call, keeping it here takes less code. If there is
607+
// ever more code between DataModel::Encode and the end of this function, it should be broken out into
608+
// TryAddResponseDataPostEncode.
595609
return FinishCommand(/* aEndDataStruct = */ false);
596610
}
597611

0 commit comments

Comments
 (0)