Skip to content

Commit

Permalink
[size-opt] Optimize DefaultStorageKeyAllocator code size (#20926)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@Damian-Nordic @pull
Damian-Nordic authored and pull[bot] committed Feb 12, 2024
1 parent 998e770 commit 1733842
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 43 deletions.
57 changes: 38 additions & 19 deletions src/app/clusters/ota-requestor/DefaultOTARequestorStorage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand All @@ -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)
Expand Down
39 changes: 21 additions & 18 deletions src/lib/support/DefaultStorageKeyAllocator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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); }
Expand All @@ -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); }
Expand All @@ -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,
Expand All @@ -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
14 changes: 8 additions & 6 deletions src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 1733842

Please sign in to comment.