From 210660102f7fb18939f1dcbf3725e05512ad50e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Tue, 19 Jul 2022 17:38:38 +0200 Subject: [PATCH] [size-opt] Optimize DefaultStorageKeyAllocator code size (#20926) 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 --- .../DefaultOTARequestorStorage.cpp | 57 ++++++++++++------- src/lib/support/DefaultStorageKeyAllocator.h | 39 +++++++------ ...enericNetworkCommissioningThreadDriver.cpp | 14 +++-- 3 files changed, 67 insertions(+), 43 deletions(-) diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestorStorage.cpp b/src/app/clusters/ota-requestor/DefaultOTARequestorStorage.cpp index 17374b43e7dc99..f79eac7fc1070e 100644 --- a/src/app/clusters/ota-requestor/DefaultOTARequestorStorage.cpp +++ b/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(writer.GetLengthWritten())); + return mPersistentStorage->SyncSetKeyValue(key.OTADefaultProviders(), buffer, static_cast(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(writer.GetLengthWritten())); + return mPersistentStorage->SyncSetKeyValue(key.OTACurrentProvider(), buffer, static_cast(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(updateToken.size())); + DefaultStorageKeyAllocator key; + + return mPersistentStorage->SyncSetKeyValue(key.OTAUpdateToken(), updateToken.data(), static_cast(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(), ¤tUpdateState, - sizeof(currentUpdateState)); + DefaultStorageKeyAllocator key; + + return mPersistentStorage->SyncSetKeyValue(key.OTACurrentUpdateState(), ¤tUpdateState, sizeof(currentUpdateState)); } CHIP_ERROR DefaultOTARequestorStorage::LoadCurrentUpdateState(OTAUpdateStateEnum & currentUpdateState) { + DefaultStorageKeyAllocator key; uint16_t size = static_cast(sizeof(currentUpdateState)); - return mPersistentStorage->SyncGetKeyValue(DefaultStorageKeyAllocator::OTACurrentUpdateState(), ¤tUpdateState, size); + + return mPersistentStorage->SyncGetKeyValue(key.OTACurrentUpdateState(), ¤tUpdateState, 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(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) diff --git a/src/lib/support/DefaultStorageKeyAllocator.h b/src/lib/support/DefaultStorageKeyAllocator.h index d87f61bb5b501f..94b315b7e37c58 100644 --- a/src/lib/support/DefaultStorageKeyAllocator.h +++ b/src/lib/support/DefaultStorageKeyAllocator.h @@ -43,7 +43,7 @@ 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); } @@ -51,18 +51,18 @@ class DefaultStorageKeyAllocator 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(nodeId >> 32), static_cast(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 diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp index 5b41aac5f2fdf2..05e34149a29da5 100644 --- a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp +++ b/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,10 +209,11 @@ 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; } @@ -219,7 +221,7 @@ CHIP_ERROR GenericThreadDriver::BackupConfiguration() // 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()