Better DecodableList with smaller binary size #39605
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Code Review
This pull request refactors list handling by unifying DecodableList to also serve as a DataModel::List, enabling DecodableType::Encode. The code replaces manual iterator loops with a lambda-based Iterate method and updates the API for DecodableList::ComputeSize. Generated code from ZAP templates has been updated to reflect this unification.
...les/energy-management-app/energy-management-common/energy-evse/src/ChargingTargetsMemMgr.cpp
Show resolved
Hide resolved
src/app/clusters/access-control-server/access-control-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/device-energy-management-server/device-energy-management-server.cpp
Show resolved
Hide resolved
src/app/clusters/device-energy-management-server/device-energy-management-server.cpp
Outdated
Show resolved
Hide resolved
| */ | ||
| CHIP_ERROR ComputeSize(size_t * size) const | ||
| CHIP_ERROR ComputeSize(size_t & size) const |
There was a problem hiding this comment.
The ComputeSize method now takes a size_t& instead of size_t*.
src/app/data-model/DecodableList.h
Outdated
Comment on lines
474
to
370
| template <typename X> | ||
| inline CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, DecodableList<X> list) | ||
| { | ||
| TLV::TLVType type; | ||
|
|
||
| ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Array, type)); | ||
| CHIP_ERROR result = list.Iterate([&writer](const auto & element, bool &) -> CHIP_ERROR { | ||
| ReturnErrorOnFailure(Encode(writer, TLV::AnonymousTag(), element)); | ||
| return CHIP_NO_ERROR; | ||
| }); | ||
| ReturnErrorOnFailure(result); | ||
| return writer.EndContainer(type); | ||
| } | ||
|
|
||
| template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true> | ||
| inline CHIP_ERROR EncodeForWrite(TLV::TLVWriter & writer, TLV::Tag tag, DecodableList<X> list) | ||
| { | ||
| TLV::TLVType type; | ||
|
|
||
| ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Array, type)); | ||
| CHIP_ERROR result = list.Iterate([&writer](const auto & element, bool &) -> CHIP_ERROR { | ||
| ReturnErrorOnFailure(EncodeForWrite(writer, TLV::AnonymousTag(), element)); | ||
| return CHIP_NO_ERROR; | ||
| }); | ||
| ReturnErrorOnFailure(result); | ||
| return writer.EndContainer(type); | ||
| } | ||
|
|
||
| template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true> | ||
| inline CHIP_ERROR EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricIndex accessingFabricIndex, DecodableList<X> list) | ||
| { | ||
| TLV::TLVType type; | ||
|
|
||
| ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Array, type)); | ||
| CHIP_ERROR result = list.Iterate([&writer](const auto & element, bool &) -> CHIP_ERROR { | ||
| ReturnErrorOnFailure(EncodeForRead(writer, TLV::AnonymousTag(), element)); | ||
| return CHIP_NO_ERROR; | ||
| }); | ||
| ReturnErrorOnFailure(result); | ||
| return writer.EndContainer(type); | ||
| } |
There was a problem hiding this comment.
The code adds Encode, EncodeForWrite, and EncodeForRead free functions for DecodableList<X>.
2be79f2 to
0e15166
Compare
|
PR #39605: Size comparison from f3fc26a to 0e15166 Increases above 0.2%:
Full report (3 builds for cc32xx, stm32)
|
0e15166 to
ab621cc
Compare
|
PR #39605: Size comparison from f3fc26a to ab621cc Increases above 0.2%:
Full report (3 builds for cc32xx, stm32)
|
ab621cc to
a321223
Compare
a321223 to
f3042ab
Compare
|
PR #39605: Size comparison from 9b7e3cf to f3042ab Increases above 0.2%:
Full report (3 builds for cc32xx, stm32)
|
b3b6c7d to
2780215
Compare
|
PR #39605: Size comparison from 861fcd5 to 2780215 Full report (9 builds for cc13x4_26x4, cc32xx, qpg, stm32)
|
2780215 to
805791d
Compare
There was a problem hiding this comment.
Between the breaking API changes and the codesize increases, the question becomes: what are the use cases? When do you want to "encode a decoded struct"? Typically encode happens on the sending end and decode on the receiving end. Bridges might need to do this, but they generally need to transcode various other things too.....
If all we need to do is address that use case, though, wouldn't adding Encode to DecodableType and then writing an overload of DataModel::Encode for DecodableList be sufficient?
| } | ||
|
|
||
| template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true> | ||
| inline CHIP_ERROR EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricIndex accessingFabricIndex, DecodableList<X> list) |
There was a problem hiding this comment.
This should be checking that accessingFabricIndex matches list, no?
There was a problem hiding this comment.
🤷♂️ List<T> doesn't - should it? (we may as well address both in this PR, as it's just a clean-up PR now)
src/app/clusters/access-control-server/access-control-server.cpp
Outdated
Show resolved
Hide resolved
805791d to
5704811
Compare
It's not just the Generating a single type whether a structure has a list or not seems, in general, beneficial - if I can get the binary size to not increase (getting close) |
5704811 to
058b184
Compare
gmarcosb
commented
Jun 26, 2025
04074a3 to
6f7c8aa
Compare
6f7c8aa to
bd10023
Compare
|
PR #39605: Size comparison from 13064a5 to bd10023 Full report (9 builds for cc13x4_26x4, cc32xx, qpg, stm32)
|
198f655 to
37a38a0
Compare
|
PR #39605: Size comparison from a8817cc to 37a38a0 Full report (7 builds for cc13x4_26x4, cc32xx, stm32)
|
3ecfd77 to
1d3be86
Compare
|
PR #39605: Size comparison from a8817cc to 1d3be86 Full report (17 builds for cc13x4_26x4, cc32xx, qpg, stm32, telink)
|
1d3be86 to
b88bc6b
Compare
gmarcosb
commented
Jun 26, 2025
| CHIP_ERROR | ||
| ChargingTargetsMemMgr::AllocAndCopy(const DataModel::List<const Structs::ChargingTargetStruct::Type> & chargingTargets) | ||
| { | ||
| // NOTE: ChargingTargetsMemMgr::PrepareDaySchedule() must be called as specified in the class comments in |
There was a problem hiding this comment.
This is a good demonstration of the benefit we get being able to iterate List<T> & DecodableList<T> in the same way
|
PR #39605: Size comparison from a8817cc to b88bc6b Full report (3 builds for cc32xx, stm32)
|
8fd522d to
6f52717
Compare
|
PR #39605: Size comparison from a8817cc to 6f52717 Increases above 0.2%:
Full report (28 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
…he learnings & size decrease from experiment
6f52717 to
c1bc532
Compare
|
PR #39605: Size comparison from 825f619 to c1bc532 Increases above 0.2%:
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
This was referenced Jun 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Originally an attempt to combine
DecodableType&Typewhen there are lists in the struct; failed due to significant binary size increaseTake some of the learnings from the experiment:
for_eachto make iteration consistent withListTesting
CI