Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert persistent storage to sync calls and binary data to align with KVS style storage #6286

Merged

Conversation

andy31415
Copy link
Contributor

Problem

Persistent storage logic is split - sync and async callbacks, the actual async callback is mostly unused. Binary data cannot be stored and all strings are assumed ascii (or at least null terminated).

Summary of Changes

  • Only keep sync versions of get/set/delete from persistent storage
  • only allow binary data read/write for persistent storage
  • remove the delegate callback from persistent storage

@todo
Copy link

todo bot commented Apr 26, 2021

no need to base-64 the serialized values AGAIN

// TODO: no need to base-64 the serialized values AGAIN
PERSISTENT_KEY_OP(device->GetDeviceId(), kPairedDeviceKeyPrefix, key,
mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner)));
}
}


This comment was generated by todo based on a TODO comment in 839f6c4 in #6286. cc @andy31415.

@todo
Copy link

todo bot commented Apr 26, 2021

no need to base64 again the value

// TODO: no need to base64 again the value
PERSISTENT_KEY_OP(static_cast<uint64_t>(0), kPairedDeviceListKeyPrefix, key,
mStorageDelegate->SyncSetKeyValue(key, value, static_cast<uint16_t>(strlen(value))));
mPairedDevicesUpdated = false;
}
chip::Platform::MemoryFree(serialized);
}
mStorageDelegate->SyncSetKeyValue(kNextAvailableKeyID, &mNextKeyId, sizeof(mNextKeyId));
}
}


This comment was generated by todo based on a TODO comment in 839f6c4 in #6286. cc @andy31415.

@todo
Copy link

todo bot commented Apr 26, 2021

ideally the error from the dispatch should be returned

// TODO: ideally the error from the dispatch should be returned
// however we expect to replace the storage delegate with KVS so for now
// we return no error (return used to be void due to async dispatch anyway)
return CHIP_NO_ERROR;
}
CHIP_ERROR CHIPPersistentStorageDelegateBridge::SyncDeleteKeyValue(const char * key)
{
NSString * keyString = [NSString stringWithUTF8String:key];
dispatch_async(mWorkQueue, ^{


This comment was generated by todo based on a TODO comment in 839f6c4 in #6286. cc @andy31415.

@todo
Copy link

todo bot commented Apr 26, 2021

ideally the error from the dispatch should be returned

// TODO: ideally the error from the dispatch should be returned
// however we expect to replace the storage delegate with KVS so for now
// we return no error (return used to be void due to async dispatch anyway)
return CHIP_NO_ERROR;
}


This comment was generated by todo based on a TODO comment in 839f6c4 in #6286. cc @andy31415.

@andy31415 andy31415 changed the title 01 cleanup persistent storage Convert persistent storage to sync calls and binary data to align with KVS style storage Apr 26, 2021
@msandstedt msandstedt self-requested a review April 28, 2021 17:09
@andy31415
Copy link
Contributor Author

#6272 (review)

Replied there and also followed up offline. Generally:

  • Agree that async is more flexible. Thinking we can create an IO thread for whenever we need async behaviour
  • current goal is to help untangle the persistent storage story (have configurationamanger, kvs, storage delegate, then have sync and async in various implementation state, have async callbacks not used)

This PR is starting to consolidate the APIs up to a point of at least having a uniform API surface for storage

Future PRs expected with:
- consolidate into one storage class (I expect KVS and replace storage delegate and configuration manager)
- move KVS outside of platform (Android does not like that) and provide pluggability
- add IO thread for async storage needs
- start taking advantage of ability of binary storage - do not double-base64 anymore and look into some encoding/decoding uniformity (have at least header, versioning and endianess flags) for binary data

@andy31415
Copy link
Contributor Author

Also, this does not compile on Darwin, in DefaultUtils.m around line 115.

// MARK: CHIPPersistentStorageDelegate
- (void)CHIPGetKeyValue:(NSString *)key handler:(SendKeyValue)completionHandler

It's not recognizing the SendKeyValue type, because its type definition was removed (in CHIPPersistentStorageDelegate.h),

removed the completion-handle key value getter.

@@ -47,13 +37,13 @@ typedef void (^CHIPSendDeleteStatus)(NSString * key, NSError * status);
* Set the value of the key to the given value
*
*/
- (void)CHIPSetKeyValue:(NSString *)key value:(NSString *)value handler:(CHIPSendSetStatus)completionHandler;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be a a dispatch async callback on Darwin, doesn't match Darwin style otherwise

@vivien-apple vivien-apple removed their request for review April 30, 2021 17:39
Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some further review, this seems like a good step forward. We can fix async later :)

@github-actions
Copy link

github-actions bot commented May 5, 2021

Size increase report for "esp32-example-build" from a8226ab

File Section File VM
chip-all-clusters-app.elf .flash.text -100 -100
chip-all-clusters-app.elf .flash.rodata -104 -104
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
[Unmapped],0,104
.xt.prop._ZTVN4chip24LifetimePersistedCounterE,0,3
.debug_aranges,0,-40
.xt.lit._ZN4chip25PersistentStorageDelegate15SyncGetKeyValueEPKcPcRt,0,-48
.debug_loc,0,-70
.debug_ranges,0,-88
.xt.prop._ZN4chip25PersistentStorageDelegate15SyncGetKeyValueEPKcPcRt,0,-88
.symtab,0,-96
.flash.text,-100,-100
.debug_abbrev,0,-104
.flash.rodata,-104,-104
.debug_frame,0,-120
.shstrtab,0,-136
.strtab,0,-296
.debug_line,0,-343
.debug_str,0,-584
.debug_info,0,-6182

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize


@github-actions
Copy link

github-actions bot commented May 5, 2021

Size increase report for "nrfconnect-example-build" from a8226ab

File Section File VM
chip-lock.elf device_handles -8 -8
chip-lock.elf text -88 -88
chip-lock.elf rodata -104 -104
chip-lighting.elf device_handles -8 -8
chip-lighting.elf text -88 -88
chip-lighting.elf rodata -104 -104
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_line,0,1
.debug_abbrev,0,-26
.debug_str,0,-584
.debug_info,0,-4239

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
device_handles,-8,-8
.debug_aranges,0,-40
.debug_ranges,0,-64
.debug_abbrev,0,-71
text,-88,-88
rodata,-104,-104
.debug_frame,0,-120
.symtab,0,-208
.debug_line,0,-252
.strtab,0,-296
.debug_loc,0,-384
.debug_str,0,-584
.debug_info,0,-6613

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
device_handles,-8,-8
.debug_aranges,0,-40
.debug_ranges,0,-64
.debug_abbrev,0,-71
text,-88,-88
rodata,-104,-104
.debug_frame,0,-120
.symtab,0,-208
.debug_line,0,-254
.strtab,0,-296
.debug_loc,0,-384
.debug_str,0,-584
.debug_info,0,-6627


@andy31415 andy31415 merged commit 2024923 into project-chip:master May 5, 2021
@andy31415 andy31415 deleted the 01_cleanup_persistent_storage branch October 28, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants