Move persistence from out of band to in-band#399
Open
ivmarkov wants to merge 3 commits intoproject-chip:mainfrom
Open
Move persistence from out of band to in-band#399ivmarkov wants to merge 3 commits intoproject-chip:mainfrom
ivmarkov wants to merge 3 commits intoproject-chip:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the persistence layer in rs-matter by introducing a new KvBlobStore trait and a transactional PersistTransaction utility to handle state persistence more robustly. It replaces the previous file-based PSM with a more granular key-value store approach, updates various cluster handlers to use this new persistence mechanism, and migrates several synchronous cluster handlers to asynchronous ones. My feedback highlights the importance of ensuring consistent transaction scoping and error handling during the commit phase of these new persistence operations to maintain data integrity.
ivmarkov
force-pushed
the
inband-persistence
branch
3 times, most recently
from
8ec0b49 to
721c35e
Compare
|
PR #399: Size comparison from 8f3d2cc to 721c35e Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
2 times, most recently
from
1d909a0 to
3fe7d1f
Compare
|
PR #399: Size comparison from 8f3d2cc to 3fe7d1f Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
from
3fe7d1f to
06cdc7e
Compare
|
PR #399: Size comparison from 8f3d2cc to 06cdc7e Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
Contributor
Author
ivmarkov
force-pushed
the
inband-persistence
branch
from
06cdc7e to
6ef5b43
Compare
|
PR #399: Size comparison from 8f3d2cc to 6ef5b43 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
from
6ef5b43 to
159230f
Compare
|
PR #399: Size comparison from 8f3d2cc to 159230f Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
from
159230f to
94506e5
Compare
|
PR #399: Size comparison from c4f1291 to 94506e5 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #399: Size comparison from c4f1291 to 1952d23 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
from
1952d23 to
e015ede
Compare
|
PR #399: Size comparison from c4f1291 to e015ede Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #399: Size comparison from c4f1291 to e457ffa Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
2 times, most recently
from
44b31a9 to
3cb42dd
Compare
|
PR #399: Size comparison from c4f1291 to 3cb42dd Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
Contributor
Author
|
This PR now did cherry-pick the optimizations from #366 which are basically "use the This did result in a small size decrease: #399 (comment) (i.e. from 9.8% increase down to 9.2% for the most badly affected thumbv6m-none-eabi target) The PR also introduced and uses a new trick - Still, all these reduced the introduced code bloat by only 20% compared to a straightforward usage of the |
|
PR #399: Size comparison from c4f1291 to 93ebab5 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
inband-persistence
branch
from
309bdbc to
4c182df
Compare
|
PR #399: Size comparison from c4f1291 to 4c182df Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
Experiment with delayed_ready and impl Future syntax in an effort to reduce the code bloat Expose the wireless networks Make it easier to integrate sequential_map downstream
ivmarkov
force-pushed
the
inband-persistence
branch
from
4c182df to
b2427f6
Compare
|
PR #399: Size comparison from d5ad8ed to b2427f6 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
Contributor
Author
|
I've now built a stage2 rustc from this much anticipated PR (this hash) and compared The TL;DR is: I don't see an enormous reduction in neither BSS nor FLASH size (I expected a bit the latter as well due to this comment):
With that said, the code-patterns "-Z pack-coroutine-layout=captures-only" is supposed to optimize (i.e. single-line async delegates using the Comparisons below; "inband-persistence" is basically this PR. "inband-persistence-sync" is a variation of this PR where the inband persistence API ( Branch "inband-persistence"Target cargo size --release --target thumbv6m-none-eabi#135527 + nightly: Target cargo size --release --target thumbv7em-none-eabi#135527 + nightly: Branch "inband-persistence-sync"Target cargo size --release --target thumbv7em-none-eabi#135527 + nightly: Target cargo size --release --target thumbv6m-none-eabi#135527 + nightly: #135527 + |
Contributor
Author
|
baa4728 reverts to a non-async API for The non-async |
|
PR #399: Size comparison from d5ad8ed to baa4728 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #399: Size comparison from d5ad8ed to bcb4893 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Background
Currently, the persistence metaphor of
rs-matteris thatrs-mattercannot really persist itself.Instead, it can tell interested parties whether it was changed (is dirty).
This "dirty" tracking mechanism allows for the implementation of custom persistence by just having the persister listening on the "dirty" status of
rs-matterand then persisting the fabrics, basic info settings and networks when they get changed / "dirty".An interesting property of such an implementation is that - by necessity - it is out of band persistence.
What that means is that when - say - a new fabric had been commissioned into the device, that fabric might had not yet been saved to the persistent storage of the device at the time when the device communicates to the commissioner a success. At that time, the fabrics become "changed" / "dirty", but only later the async persister might trigger and flush the fabric to the persistent storage.
This type of implementation of persistence was a necessity back in the early days of
rs-matter, when Rust did not support (yet) async traits. And we DO want the persister to be an async trait, because on embedded baremetal, all interesting persisters are async (sequential-map,ekv), even if the access to the NOR FLASH is not necessarily async.What this PR does
As the name suggests, the PR switches the persistence of
rs-matterto a regular in-band one, wherers-mattertakes an async persistence trait impl -KvBlobStore- and then calls the persistence trait as-necessary from the NOC, Wifi, Thread, Basic Info, Gen Comm and other clusters that change any of the rs-matter values which are subject to persistence (fabrics, basic info settings and wireless networks).But why?
First of all, I'm not necessarily convinced that this in-band persistence is an obvious huge win over the existing out-of-band one. For one, it converts even more system cluster handlers from non-async to async:
... basically all clusters that modify non-volatile data now need to be async, so that they can call the persister on successful modification.
... and as we know from #366 , async cluster handlers do not have 0% overhead.
On the other hand, an in-band persistence has the following benefits:
rs-matterto aKvBlobStoretrait in downstreamrs-matter-stack/embassy/esp-idfanyway. So why not push theKvBlobStoreupstream tors-matter?Last but not least, switching to in-band storage makes fixing sysgrok/rs-matter-stack#27 very easy, as this PR does.