From 9b8fffe0bb4e7ba7aac319f5905070a3476db7cb Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Thu, 16 Jan 2025 20:20:42 -0500
Subject: [PATCH] Reduce the amount of per-instantiation logic in
 EncodeListItem. (#37093)

The parts that are not dependent on ItemType can be separate functions.
---
 src/app/AttributeValueEncoder.cpp | 29 +++++++++++++++++++++
 src/app/AttributeValueEncoder.h   | 42 ++++++++++++++-----------------
 2 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/src/app/AttributeValueEncoder.cpp b/src/app/AttributeValueEncoder.cpp
index 4b6bb10e66abdb..b8f2540e83a4c5 100644
--- a/src/app/AttributeValueEncoder.cpp
+++ b/src/app/AttributeValueEncoder.cpp
@@ -109,5 +109,34 @@ void AttributeValueEncoder::EnsureListEnded()
     }
 }
 
+bool AttributeValueEncoder::ShouldEncodeListItem(TLV::TLVWriter & aCheckpoint)
+{
+    // EncodeListItem (our caller) must be called after EnsureListStarted(),
+    // thus mCurrentEncodingListIndex and mEncodeState.mCurrentEncodingListIndex
+    // are not invalid values.
+    if (mCurrentEncodingListIndex < mEncodeState.CurrentEncodingListIndex())
+    {
+        // We have encoded this element in previous chunks, skip it.
+        mCurrentEncodingListIndex++;
+        return false;
+    }
+
+    mAttributeReportIBsBuilder.Checkpoint(aCheckpoint);
+    return true;
+}
+
+void AttributeValueEncoder::PostEncodeListItem(CHIP_ERROR aEncodeStatus, const TLV::TLVWriter & aCheckpoint)
+{
+    if (aEncodeStatus != CHIP_NO_ERROR)
+    {
+        mAttributeReportIBsBuilder.Rollback(aCheckpoint);
+        return;
+    }
+
+    mCurrentEncodingListIndex++;
+    mEncodeState.SetCurrentEncodingListIndex(mCurrentEncodingListIndex);
+    mEncodedAtLeastOneListItem = true;
+}
+
 } // namespace app
 } // namespace chip
diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h
index d90e0ee4566753..39bbe9cb03545b 100644
--- a/src/app/AttributeValueEncoder.h
+++ b/src/app/AttributeValueEncoder.h
@@ -55,17 +55,22 @@ class AttributeValueEncoder
             VerifyOrReturnError(!mAttributeValueEncoder.mIsFabricFiltered ||
                                     aArg.GetFabricIndex() == mAttributeValueEncoder.AccessingFabricIndex(),
                                 CHIP_NO_ERROR);
-            return mAttributeValueEncoder.EncodeListItem(aArg, mAttributeValueEncoder.AccessingFabricIndex());
+            return mAttributeValueEncoder.EncodeListItem(mCheckpoint, aArg, mAttributeValueEncoder.AccessingFabricIndex());
         }
 
         template <typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true>
         CHIP_ERROR Encode(const T & aArg) const
         {
-            return mAttributeValueEncoder.EncodeListItem(aArg);
+            return mAttributeValueEncoder.EncodeListItem(mCheckpoint, aArg);
         }
 
     private:
         AttributeValueEncoder & mAttributeValueEncoder;
+        // Avoid calling the TLVWriter constructor for every instantiation of
+        // EncodeListItem.  We treat encoding as a const operation, so either
+        // have to put this on the stack (in which case it's per-instantiation),
+        // or have it as mutable state.
+        mutable TLV::TLVWriter mCheckpoint;
     };
 
     AttributeValueEncoder(AttributeReportIBs::Builder & aAttributeReportIBsBuilder, Access::SubjectDescriptor subjectDescriptor,
@@ -163,25 +168,26 @@ class AttributeValueEncoder
     friend class ListEncodeHelper;
     friend class TestOnlyAttributeValueEncoderAccessor;
 
+    // Returns true if the list item should be encoded.  If it should, the
+    // passed-in TLVWriter will be used to checkpoint the current state of our
+    // attribute report list builder.
+    bool ShouldEncodeListItem(TLV::TLVWriter & aCheckpoint);
+
+    // Does any cleanup work needed after attempting to encode a list item.
+    void PostEncodeListItem(CHIP_ERROR aEncodeStatus, const TLV::TLVWriter & aCheckpoint);
+
     // EncodeListItem may be given an extra FabricIndex argument as a second
     // arg, or not.  Represent that via a parameter pack (which might be
     // empty). In practice, for any given ItemType the extra arg is either there
     // or not, so we don't get more template explosion due to aExtraArgs.
     template <typename ItemType, typename... ExtraArgTypes>
-    CHIP_ERROR EncodeListItem(const ItemType & aItem, ExtraArgTypes &&... aExtraArgs)
+    CHIP_ERROR EncodeListItem(TLV::TLVWriter & aCheckpoint, const ItemType & aItem, ExtraArgTypes &&... aExtraArgs)
     {
-        // EncodeListItem must be called after EnsureListStarted(), thus mCurrentEncodingListIndex and
-        // mEncodeState.mCurrentEncodingListIndex are not invalid values.
-        if (mCurrentEncodingListIndex < mEncodeState.CurrentEncodingListIndex())
+        if (!ShouldEncodeListItem(aCheckpoint))
         {
-            // We have encoded this element in previous chunks, skip it.
-            mCurrentEncodingListIndex++;
             return CHIP_NO_ERROR;
         }
 
-        TLV::TLVWriter backup;
-        mAttributeReportIBsBuilder.Checkpoint(backup);
-
         CHIP_ERROR err;
         if (mEncodingInitialList)
         {
@@ -194,19 +200,9 @@ class AttributeValueEncoder
         {
             err = EncodeAttributeReportIB(aItem, std::forward<ExtraArgTypes>(aExtraArgs)...);
         }
-        if (err != CHIP_NO_ERROR)
-        {
-            // For list chunking, ReportEngine should not rollback the buffer when CHIP_ERROR_NO_MEMORY or similar error occurred.
-            // However, the error might be raised in the middle of encoding procedure, then the buffer may contain partial data,
-            // unclosed containers etc. This line clears all possible partial data and makes EncodeListItem is atomic.
-            mAttributeReportIBsBuilder.Rollback(backup);
-            return err;
-        }
 
-        mCurrentEncodingListIndex++;
-        mEncodeState.SetCurrentEncodingListIndex(mCurrentEncodingListIndex);
-        mEncodedAtLeastOneListItem = true;
-        return CHIP_NO_ERROR;
+        PostEncodeListItem(err, aCheckpoint);
+        return err;
     }
 
     /**