Encode as much of a list as possible as a single IB when reading. (#2…

…6286)

Instead of doing one IB to start empty list and one IB per item, switch to doing
one IB with as many items as we can fit in our packet (with a REPLACE path),
then one IB per item with ADD paths as now.

This reduces the data transferred for a wildcard-everything read from
lighting-app from 26 AttributeReport packets to 12.

Fixes #26271

src/app/AttributeAccessInterface.cpp

@@ -55,39 +55,82 @@ CHIP_ERROR AttributeReportBuilder::FinishAttribute(AttributeReportIBs::Builder &
     return aAttributeReportIBsBuilder.GetAttributeReport().EndOfAttributeReportIB().GetError();
 }
 
+namespace {
+
+constexpr uint32_t kEndOfListByteCount = 1;
+// 2 bytes: one to end the AttributeDataIB and one to end the AttributeReportIB.
+constexpr uint32_t kEndOfAttributeReportIBByteCount = 2;
+constexpr TLV::TLVType kAttributeDataIBType         = TLV::kTLVType_Structure;
+
+} // anonymous namespace
+
 CHIP_ERROR AttributeValueEncoder::EnsureListStarted()
 {
-    if (mCurrentEncodingListIndex == kInvalidListIndex)
+    VerifyOrDie(mCurrentEncodingListIndex == kInvalidListIndex);
+
+    mEncodingInitialList = (mEncodeState.mCurrentEncodingListIndex == kInvalidListIndex);
+    if (mEncodingInitialList)
     {
-        if (mEncodeState.mCurrentEncodingListIndex == kInvalidListIndex)
-        {
-            // Clear mAllowPartialData flag here since this encode procedure is not atomic.
-            // The most common error in this function is CHIP_ERROR_NO_MEMORY / CHIP_ERROR_BUFFER_TOO_SMALL, just revert and try
-            // next time is ok.
-            mEncodeState.mAllowPartialData = false;
-            // Spec 10.5.4.3.1, 10.5.4.6 (Replace a list w/ Multiple IBs)
-            // Put an empty array before encoding the first array element for list chunking.
-            AttributeReportBuilder builder;
-
-            mPath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;
-            ReturnErrorOnFailure(builder.PrepareAttribute(mAttributeReportIBsBuilder, mPath, mDataVersion));
-            ReturnErrorOnFailure(builder.EncodeValue(mAttributeReportIBsBuilder, DataModel::List<uint8_t>()));
-
-            ReturnErrorOnFailure(builder.FinishAttribute(mAttributeReportIBsBuilder));
-            mEncodeState.mCurrentEncodingListIndex = 0;
-        }
-        mCurrentEncodingListIndex = 0;
+        // Clear mAllowPartialData flag here since this encode procedure is not atomic.
+        // The most common error in this function is CHIP_ERROR_NO_MEMORY / CHIP_ERROR_BUFFER_TOO_SMALL, just revert and try
+        // next time is ok.
+        mEncodeState.mAllowPartialData = false;
+
+        AttributeReportBuilder builder;
+
+        mPath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;
+        ReturnErrorOnFailure(builder.PrepareAttribute(mAttributeReportIBsBuilder, mPath, mDataVersion));
+
+        auto * attributeDataWriter = mAttributeReportIBsBuilder.GetAttributeReport().GetAttributeData().GetWriter();
+        TLV::TLVType outerType;
+        ReturnErrorOnFailure(
+            attributeDataWriter->StartContainer(TLV::ContextTag(AttributeDataIB::Tag::kData), TLV::kTLVType_Array, outerType));
+        VerifyOrDie(outerType == kAttributeDataIBType);
+
+        // Instead of reserving hardcoded amounts, we could checkpoint the
+        // writer, encode array end and FinishAttribute, check that this fits,
+        // measure how much the writer advanced, then restore the checkpoint,
+        // reserve the measured value, and save it.  But that's probably more
+        // cycles than just reserving this known constant.
+        ReturnErrorOnFailure(
+            mAttributeReportIBsBuilder.GetWriter()->ReserveBuffer(kEndOfAttributeReportIBByteCount + kEndOfListByteCount));
+
+        mEncodeState.mCurrentEncodingListIndex = 0;
     }
+    else
+    {
+        // For all elements in the list, a report with append operation will be generated. This will not be changed during encoding
+        // of each report since the users cannot access mPath.
+        mPath.mListOp = ConcreteDataAttributePath::ListOperation::AppendItem;
+    }
+
+    mCurrentEncodingListIndex = 0;
 
-    // After encoding the empty list, the remaining items are atomically encoded into the buffer. Tell report engine to not
+    // After encoding the initial list start, the remaining items are atomically encoded into the buffer. Tell report engine to not
     // revert partial data.
     mEncodeState.mAllowPartialData = true;
 
-    // For all elements in the list, a report with append operation will be generated. This will not be changed during encoding
-    // of each report since the users cannot access mPath.
-    mPath.mListOp = ConcreteDataAttributePath::ListOperation::AppendItem;
     return CHIP_NO_ERROR;
 }
 
+void AttributeValueEncoder::EnsureListEnded()
+{
+    if (!mEncodingInitialList)
+    {
+        // Nothing to do.
+        return;
+    }
+
+    // Unreserve the space we reserved just for this.  Crash if anything here
+    // fails, because that would mean that we've corrupted our data, and since
+    // mEncodeState.mAllowPartialData is true nothing will clean up for us here.
+    auto * attributeDataWriter = mAttributeReportIBsBuilder.GetAttributeReport().GetAttributeData().GetWriter();
+    VerifyOrDie(attributeDataWriter->UnreserveBuffer(kEndOfListByteCount + kEndOfAttributeReportIBByteCount) == CHIP_NO_ERROR);
+    VerifyOrDie(attributeDataWriter->EndContainer(kAttributeDataIBType) == CHIP_NO_ERROR);
+
+    AttributeReportBuilder builder;
+    VerifyOrDie(builder.FinishAttribute(mAttributeReportIBsBuilder) == CHIP_NO_ERROR);
+}
+
 } // namespace app
 } // namespace chip

src/app/AttributeAccessInterface.h

@@ -77,18 +77,18 @@ class AttributeReportBuilder
      * EncodeValue encodes the value field of the report, it should be called exactly once.
      */
     template <typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true, typename... Ts>
-    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, T && item, Ts &&... aArgs)
+    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, TLV::Tag tag, T && item, Ts &&... aArgs)
     {
-        return DataModel::Encode(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()),
-                                 TLV::ContextTag(AttributeDataIB::Tag::kData), item, std::forward<Ts>(aArgs)...);
+        return DataModel::Encode(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()), tag, item,
+                                 std::forward<Ts>(aArgs)...);
     }
 
     template <typename T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, bool> = true, typename... Ts>
-    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, FabricIndex accessingFabricIndex, T && item,
-                           Ts &&... aArgs)
+    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, TLV::Tag tag, FabricIndex accessingFabricIndex,
+                           T && item, Ts &&... aArgs)
     {
-        return DataModel::EncodeForRead(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()),
-                                        TLV::ContextTag(AttributeDataIB::Tag::kData), accessingFabricIndex, item);
+        return DataModel::EncodeForRead(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()), tag,
+                                        accessingFabricIndex, item, std::forward<Ts>(aArgs)...);
     }
 };
 
@@ -224,10 +224,19 @@ class AttributeValueEncoder
         // An empty list is encoded iff both mCurrentEncodingListIndex and mEncodeState.mCurrentEncodingListIndex are invalid
         // values. After encoding the empty list, mEncodeState.mCurrentEncodingListIndex and mCurrentEncodingListIndex are set to 0.
         ReturnErrorOnFailure(EnsureListStarted());
-        ReturnErrorOnFailure(aCallback(ListEncodeHelper(*this)));
-        // The Encode procedure finished without any error, clear the state.
-        mEncodeState = AttributeEncodeState();
-        return CHIP_NO_ERROR;
+        CHIP_ERROR err = aCallback(ListEncodeHelper(*this));
+
+        // Even if encoding list items failed, make sure we EnsureListEnded().
+        // Since we encode list items atomically, in the case when we just
+        // didn't fit the next item we want to make sure our list is properly
+        // ended before the reporting engine starts chunking.
+        EnsureListEnded();
+        if (err == CHIP_NO_ERROR)
+        {
+            // The Encode procedure finished without any error, clear the state.
+            mEncodeState = AttributeEncodeState();
+        }
+        return err;
     }
 
     bool TriedEncode() const { return mTriedEncode; }
@@ -261,7 +270,17 @@ class AttributeValueEncoder
         TLV::TLVWriter backup;
         mAttributeReportIBsBuilder.Checkpoint(backup);
 
-        CHIP_ERROR err = EncodeAttributeReportIB(std::forward<Ts>(aArgs)...);
+        CHIP_ERROR err;
+        if (mEncodingInitialList)
+        {
+            // Just encode a single item, with an anonymous tag.
+            AttributeReportBuilder builder;
+            err = builder.EncodeValue(mAttributeReportIBsBuilder, TLV::AnonymousTag(), std::forward<Ts>(aArgs)...);
+        }
+        else
+        {
+            err = EncodeAttributeReportIB(std::forward<Ts>(aArgs)...);
+        }
         if (err != CHIP_NO_ERROR)
         {
             // For list chunking, ReportEngine should not rollback the buffer when CHIP_ERROR_NO_MEMORY or similar error occurred.
@@ -288,32 +307,39 @@ class AttributeValueEncoder
     CHIP_ERROR EncodeAttributeReportIB(Ts &&... aArgs)
     {
         AttributeReportBuilder builder;
-
         ReturnErrorOnFailure(builder.PrepareAttribute(mAttributeReportIBsBuilder, mPath, mDataVersion));
-        ReturnErrorOnFailure(builder.EncodeValue(mAttributeReportIBsBuilder, std::forward<Ts>(aArgs)...));
+        ReturnErrorOnFailure(builder.EncodeValue(mAttributeReportIBsBuilder, TLV::ContextTag(AttributeDataIB::Tag::kData),
+                                                 std::forward<Ts>(aArgs)...));
 
         return builder.FinishAttribute(mAttributeReportIBsBuilder);
     }
 
     /**
-     * EnsureListStarted encodes the first item of one report with lists (an
-     * empty list), as needed.
+     * EnsureListStarted sets our mCurrentEncodingListIndex to 0, and:
      *
-     * If internal state indicates we have already encoded the empty list, this function will encode nothing, set
-     * mCurrentEncodingListIndex to 0 and return CHIP_NO_ERROR.
+     * * If we are just starting the list, gets us ready to encode list items.
      *
-     * In all cases this function guarantees that mPath.mListOp is AppendItem
-     * after it returns, because at that point we will be encoding the list
-     * items.
+     * * If we are continuing a chunked list, guarantees that mPath.mListOp is
+     *   AppendItem after it returns.
      */
     CHIP_ERROR EnsureListStarted();
 
+    /**
+     * EnsureListEnded writes out the end of the list and our attribute data IB,
+     * if we were encoding our initial list
+     */
+    void EnsureListEnded();
+
     bool mTriedEncode = false;
     AttributeReportIBs::Builder & mAttributeReportIBsBuilder;
     const FabricIndex mAccessingFabricIndex;
     ConcreteDataAttributePath mPath;
     DataVersion mDataVersion;
     bool mIsFabricFiltered = false;
+    // mEncodingInitialList is true if we're encoding a list and we have not
+    // started chunking it yet, so we're encoding a single attribute report IB
+    // for the whole list, not one per item.
+    bool mEncodingInitialList = false;
     AttributeEncodeState mEncodeState;
     ListIndex mCurrentEncodingListIndex = kInvalidListIndex;
 };