[size-opt] Optimize DefaultStorageKeyAllocator code size (#20926) (#2…

…0941)

It turns out that calling Format() for each key costs
non-negligible amount of flash while it's not really needed
for constant keys.

Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>

Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com>

src/app/clusters/ota-requestor/DefaultOTARequestorStorage.cpp

@@ -40,6 +40,7 @@ constexpr size_t kProviderListMaxSerializedSize = kProviderMaxSerializedSize * C
 
 CHIP_ERROR DefaultOTARequestorStorage::StoreDefaultProviders(const ProviderLocationList & providers)
 {
+    DefaultStorageKeyAllocator key;
     uint8_t buffer[kProviderListMaxSerializedSize];
     TLV::TLVWriter writer;
     TLV::TLVType outerType;
@@ -55,16 +56,16 @@ CHIP_ERROR DefaultOTARequestorStorage::StoreDefaultProviders(const ProviderLocat
 
     ReturnErrorOnFailure(writer.EndContainer(outerType));
 
-    return mPersistentStorage->SyncSetKeyValue(DefaultStorageKeyAllocator::OTADefaultProviders(), buffer,
-                                               static_cast<uint16_t>(writer.GetLengthWritten()));
+    return mPersistentStorage->SyncSetKeyValue(key.OTADefaultProviders(), buffer, static_cast<uint16_t>(writer.GetLengthWritten()));
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::LoadDefaultProviders(ProviderLocationList & providers)
 {
+    DefaultStorageKeyAllocator key;
     uint8_t buffer[kProviderListMaxSerializedSize];
     MutableByteSpan bufferSpan(buffer);
 
-    ReturnErrorOnFailure(Load(DefaultStorageKeyAllocator::OTADefaultProviders(), bufferSpan));
+    ReturnErrorOnFailure(Load(key.OTADefaultProviders(), bufferSpan));
 
     TLV::TLVReader reader;
     TLV::TLVType outerType;
@@ -87,27 +88,30 @@ CHIP_ERROR DefaultOTARequestorStorage::LoadDefaultProviders(ProviderLocationList
 
 CHIP_ERROR DefaultOTARequestorStorage::StoreCurrentProviderLocation(const ProviderLocationType & provider)
 {
+    DefaultStorageKeyAllocator key;
     uint8_t buffer[kProviderMaxSerializedSize];
     TLV::TLVWriter writer;
 
     writer.Init(buffer);
     ReturnErrorOnFailure(provider.EncodeForRead(writer, TLV::AnonymousTag(), provider.fabricIndex));
 
-    return mPersistentStorage->SyncSetKeyValue(DefaultStorageKeyAllocator::OTACurrentProvider(), buffer,
-                                               static_cast<uint16_t>(writer.GetLengthWritten()));
+    return mPersistentStorage->SyncSetKeyValue(key.OTACurrentProvider(), buffer, static_cast<uint16_t>(writer.GetLengthWritten()));
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::ClearCurrentProviderLocation()
 {
-    return mPersistentStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::OTACurrentProvider());
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncDeleteKeyValue(key.OTACurrentProvider());
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::LoadCurrentProviderLocation(ProviderLocationType & provider)
 {
+    DefaultStorageKeyAllocator key;
     uint8_t buffer[kProviderMaxSerializedSize];
     MutableByteSpan bufferSpan(buffer);
 
-    ReturnErrorOnFailure(Load(DefaultStorageKeyAllocator::OTACurrentProvider(), bufferSpan));
+    ReturnErrorOnFailure(Load(key.OTACurrentProvider(), bufferSpan));
 
     TLV::TLVReader reader;
 
@@ -120,52 +124,67 @@ CHIP_ERROR DefaultOTARequestorStorage::LoadCurrentProviderLocation(ProviderLocat
 
 CHIP_ERROR DefaultOTARequestorStorage::StoreUpdateToken(ByteSpan updateToken)
 {
-    return mPersistentStorage->SyncSetKeyValue(DefaultStorageKeyAllocator::OTAUpdateToken(), updateToken.data(),
-                                               static_cast<uint16_t>(updateToken.size()));
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncSetKeyValue(key.OTAUpdateToken(), updateToken.data(), static_cast<uint16_t>(updateToken.size()));
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::ClearUpdateToken()
 {
-    return mPersistentStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::OTAUpdateToken());
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncDeleteKeyValue(key.OTAUpdateToken());
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::LoadUpdateToken(MutableByteSpan & updateToken)
 {
-    return Load(DefaultStorageKeyAllocator::OTAUpdateToken(), updateToken);
+    DefaultStorageKeyAllocator key;
+
+    return Load(key.OTAUpdateToken(), updateToken);
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::StoreCurrentUpdateState(OTAUpdateStateEnum currentUpdateState)
 {
-    return mPersistentStorage->SyncSetKeyValue(DefaultStorageKeyAllocator::OTACurrentUpdateState(), &currentUpdateState,
-                                               sizeof(currentUpdateState));
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncSetKeyValue(key.OTACurrentUpdateState(), &currentUpdateState, sizeof(currentUpdateState));
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::LoadCurrentUpdateState(OTAUpdateStateEnum & currentUpdateState)
 {
+    DefaultStorageKeyAllocator key;
     uint16_t size = static_cast<uint16_t>(sizeof(currentUpdateState));
-    return mPersistentStorage->SyncGetKeyValue(DefaultStorageKeyAllocator::OTACurrentUpdateState(), &currentUpdateState, size);
+
+    return mPersistentStorage->SyncGetKeyValue(key.OTACurrentUpdateState(), &currentUpdateState, size);
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::ClearCurrentUpdateState()
 {
-    return mPersistentStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::OTACurrentUpdateState());
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncDeleteKeyValue(key.OTACurrentUpdateState());
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::StoreTargetVersion(uint32_t targetVersion)
 {
-    return mPersistentStorage->SyncSetKeyValue(DefaultStorageKeyAllocator::OTATargetVersion(), &targetVersion,
-                                               sizeof(targetVersion));
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncSetKeyValue(key.OTATargetVersion(), &targetVersion, sizeof(targetVersion));
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::LoadTargetVersion(uint32_t & targetVersion)
 {
+    DefaultStorageKeyAllocator key;
     uint16_t size = static_cast<uint16_t>(sizeof(targetVersion));
-    return mPersistentStorage->SyncGetKeyValue(DefaultStorageKeyAllocator::OTATargetVersion(), &targetVersion, size);
+
+    return mPersistentStorage->SyncGetKeyValue(key.OTATargetVersion(), &targetVersion, size);
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::ClearTargetVersion()
 {
-    return mPersistentStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::OTATargetVersion());
+    DefaultStorageKeyAllocator key;
+
+    return mPersistentStorage->SyncDeleteKeyValue(key.OTATargetVersion());
 }
 
 CHIP_ERROR DefaultOTARequestorStorage::Load(const char * key, MutableByteSpan & buffer)

src/lib/support/DefaultStorageKeyAllocator.h

@@ -43,26 +43,26 @@ class DefaultStorageKeyAllocator
     const char * KeyName() { return mKeyName; }
 
     // Fabric Table
-    const char * FabricIndexInfo() { return Format("g/fidx"); }
+    const char * FabricIndexInfo() { return SetConst("g/fidx"); }
     const char * FabricNOC(FabricIndex fabric) { return Format("f/%x/n", fabric); }
     const char * FabricICAC(FabricIndex fabric) { return Format("f/%x/i", fabric); }
     const char * FabricRCAC(FabricIndex fabric) { return Format("f/%x/r", fabric); }
     const char * FabricMetadata(FabricIndex fabric) { return Format("f/%x/m", fabric); }
     const char * FabricOpKey(FabricIndex fabric) { return Format("f/%x/o", fabric); }
 
     // Fail-safe handling
-    const char * FailSafeCommitMarkerKey() { return Format("g/fs/c"); }
-    static const char * FailSafeNetworkConfig() { return "g/fs/n"; }
+    const char * FailSafeCommitMarkerKey() { return SetConst("g/fs/c"); }
+    const char * FailSafeNetworkConfig() { return SetConst("g/fs/n"); }
 
     // LastKnownGoodTime
-    const char * LastKnownGoodTimeKey() { return Format("g/lkgt"); }
+    const char * LastKnownGoodTimeKey() { return SetConst("g/lkgt"); }
 
     // Session resumption
     const char * FabricSession(FabricIndex fabric, NodeId nodeId)
     {
         return Format("f/%x/s/%08" PRIX32 "%08" PRIX32, fabric, static_cast<uint32_t>(nodeId >> 32), static_cast<uint32_t>(nodeId));
     }
-    const char * SessionResumptionIndex() { return Format("g/sri"); }
+    const char * SessionResumptionIndex() { return SetConst("g/sri"); }
     const char * SessionResumption(const char * resumptionIdBase64) { return Format("g/s/%s", resumptionIdBase64); }
 
     // Access Control
@@ -73,8 +73,8 @@ class DefaultStorageKeyAllocator
     const char * AccessControlExtensionEntry(FabricIndex fabric) { return Format("f/%x/ac/1", fabric); }
 
     // Group Message Counters
-    const char * GroupDataCounter() { return Format("g/gdc"); }
-    const char * GroupControlCounter() { return Format("g/gcc"); }
+    const char * GroupDataCounter() { return SetConst("g/gdc"); }
+    const char * GroupControlCounter() { return SetConst("g/gcc"); }
 
     // Device Information Provider
     const char * UserLabelLengthKey(EndpointId endpoint) { return Format("g/userlbl/%x", endpoint); }
@@ -83,7 +83,7 @@ class DefaultStorageKeyAllocator
     // Group Data Provider
 
     // List of fabric indices that have endpoint-to-group associations defined.
-    const char * GroupFabricList() { return Format("g/gfl"); }
+    const char * GroupFabricList() { return SetConst("g/gfl"); }
     const char * FabricGroups(chip::FabricIndex fabric) { return Format("f/%x/g", fabric); }
     const char * FabricGroup(chip::FabricIndex fabric, chip::GroupId group) { return Format("f/%x/g/%x", fabric, group); }
     const char * FabricGroupKey(chip::FabricIndex fabric, uint16_t index) { return Format("f/%x/gk/%x", fabric, index); }
@@ -102,17 +102,17 @@ class DefaultStorageKeyAllocator
 
     // TODO: Should store fabric-specific parts of the binding list under keys
     // starting with "f/%x/".
-    const char * BindingTable() { return Format("g/bt"); }
+    const char * BindingTable() { return SetConst("g/bt"); }
     const char * BindingTableEntry(uint8_t index) { return Format("g/bt/%x", index); }
 
-    static const char * OTADefaultProviders() { return "g/o/dp"; }
-    static const char * OTACurrentProvider() { return "g/o/cp"; }
-    static const char * OTAUpdateToken() { return "g/o/ut"; }
-    static const char * OTACurrentUpdateState() { return "g/o/us"; }
-    static const char * OTATargetVersion() { return "g/o/tv"; }
+    const char * OTADefaultProviders() { return SetConst("g/o/dp"); }
+    const char * OTACurrentProvider() { return SetConst("g/o/cp"); }
+    const char * OTAUpdateToken() { return SetConst("g/o/ut"); }
+    const char * OTACurrentUpdateState() { return SetConst("g/o/us"); }
+    const char * OTATargetVersion() { return SetConst("g/o/tv"); }
 
     // Event number counter.
-    const char * IMEventNumber() { return Format("g/im/ec"); }
+    const char * IMEventNumber() { return SetConst("g/im/ec"); }
 
 protected:
     // The ENFORCE_FORMAT args are "off by one" because this is a class method,
@@ -121,13 +121,16 @@ class DefaultStorageKeyAllocator
     {
         va_list args;
         va_start(args, format);
-        vsnprintf(mKeyName, sizeof(mKeyName), format, args);
+        vsnprintf(mKeyNameBuffer, sizeof(mKeyNameBuffer), format, args);
         va_end(args);
-        return mKeyName;
+        return mKeyName = mKeyNameBuffer;
     }
 
+    const char * SetConst(const char * keyName) { return mKeyName = keyName; }
+
 private:
-    char mKeyName[PersistentStorageDelegate::kKeyLengthMax + 1] = { 0 };
+    const char * mKeyName                                             = nullptr;
+    char mKeyNameBuffer[PersistentStorageDelegate::kKeyLengthMax + 1] = { 0 };
 };
 
 } // namespace chip

src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp

@@ -69,18 +69,19 @@ CHIP_ERROR GenericThreadDriver::CommitConfiguration()
     // the backup, so that it cannot be restored. If no backup could be found, it means that the
     // configuration has not been modified since the fail-safe was armed, so return with no error.
 
-    CHIP_ERROR error = KeyValueStoreMgr().Delete(DefaultStorageKeyAllocator::FailSafeNetworkConfig());
+    DefaultStorageKeyAllocator key;
+    CHIP_ERROR error = KeyValueStoreMgr().Delete(key.FailSafeNetworkConfig());
 
     return error == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND ? CHIP_NO_ERROR : error;
 }
 
 CHIP_ERROR GenericThreadDriver::RevertConfiguration()
 {
+    DefaultStorageKeyAllocator key;
     uint8_t datasetBytes[Thread::kSizeOperationalDataset];
     size_t datasetLength;
 
-    CHIP_ERROR error = KeyValueStoreMgr().Get(DefaultStorageKeyAllocator::FailSafeNetworkConfig(), datasetBytes,
-                                              sizeof(datasetBytes), &datasetLength);
+    CHIP_ERROR error = KeyValueStoreMgr().Get(key.FailSafeNetworkConfig(), datasetBytes, sizeof(datasetBytes), &datasetLength);
 
     // If no backup could be found, it means that the network configuration has not been modified
     // since the fail-safe was armed, so return with no error.
@@ -101,7 +102,7 @@ CHIP_ERROR GenericThreadDriver::RevertConfiguration()
     ReturnErrorOnFailure(DeviceLayer::ThreadStackMgrImpl().AttachToThreadNetwork(mStagingNetwork, /* callback */ nullptr));
 
     // TODO: What happens on errors above? Why do we not remove the failsafe?
-    return KeyValueStoreMgr().Delete(DefaultStorageKeyAllocator::FailSafeNetworkConfig());
+    return KeyValueStoreMgr().Delete(key.FailSafeNetworkConfig());
 }
 
 Status GenericThreadDriver::AddOrUpdateNetwork(ByteSpan operationalDataset, MutableCharSpan & outDebugText,
@@ -208,18 +209,19 @@ Status GenericThreadDriver::MatchesNetworkId(const Thread::OperationalDataset &
 
 CHIP_ERROR GenericThreadDriver::BackupConfiguration()
 {
+    DefaultStorageKeyAllocator key;
     uint8_t dummy;
 
     // If configuration is already backed up, return with no error
-    if (KeyValueStoreMgr().Get(DefaultStorageKeyAllocator::FailSafeNetworkConfig(), &dummy, 0) == CHIP_ERROR_BUFFER_TOO_SMALL)
+    if (KeyValueStoreMgr().Get(key.FailSafeNetworkConfig(), &dummy, 0) == CHIP_ERROR_BUFFER_TOO_SMALL)
     {
         return CHIP_NO_ERROR;
     }
 
     // Not all KVS implementations support zero-length values, so use a special value in such a case.
     ByteSpan dataset = mStagingNetwork.IsEmpty() ? ByteSpan(kEmptyDataset) : mStagingNetwork.AsByteSpan();
 
-    return KeyValueStoreMgr().Put(DefaultStorageKeyAllocator::FailSafeNetworkConfig(), dataset.data(), dataset.size());
+    return KeyValueStoreMgr().Put(key.FailSafeNetworkConfig(), dataset.data(), dataset.size());
 }
 
 size_t GenericThreadDriver::ThreadNetworkIterator::Count()