Improvements to the TLV framework #206
Merged
Conversation
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
|
Review changes with SemanticDiff. Analyzed 46 of 74 files. Overall, the semantic diff is 33% smaller than the GitHub diff. File Information
|
Delay the creation of the Error type as it is expensive when backtrace is enabled
|
CI should be fine now. |
kedars
approved these changes
Sep 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This is a rather large PR, so let me explain what is going on here:
The TLV improvements introduced here can be classified in five main areas (listed in order of importance):
1. Shorten
TLVElement(and related types) from 40 bytes to 8 bytes (32 bit archs) or 16 bytes (64 bit archs)While it might look like a minimal gain, let's not forget that the size of individual TLV elements can quickly add-up in structure size. Take the IM structure
ReadReqfor example:TLVElement)WriteReq,InvokeReqetc.Worst offender:
CertCertRefthough, read on): 16 bytes (or even 8 for 32 bit archs)How?
We get smaller memory size by delaying computations.
After this PR
TLVElementis simply a newtype over&[u8]. All parsing is done post-creation of the TLVElement, when its various methods are invoked.Would that have any performance implications on an MCU?
Note that the same size reduction is now applicable for container types which also before were just wrappers over
TLVElement: likeTLVContainerand its concrete instantiationsTLVArray,TLVListandTLVStructure.2. Support for on-the-fly TLV structures (newtypes over
TLVElement)Bringing the "delay the TLV computations" idea to the extreme gives us TLV structures which are also just newtype wrappers over
TLVElement(and are thus only 16 / 8 bytes in size):ReadReqwith the newReadReqRefWriteReqwith the newWriteReqRefCert) withCertRefThe
FromTLV/ToTLVproc-macros are extended to support these "on-the-fly" TLV structures.Granted, these structures are not "self-descriptive" as the existing ones, so the user has to manually implement their fields in the form of methods. The question is though, would that be a big issue once we have the IDL framework generating these structures (and their methods) from IDL? Probably not.
3.
FromTLVextended to support an extra method -init_from_tlvThis is one application of the new "in-place-init" from the previous PR.
This is useful when e.g. de-serializing "owned" large objects, like
Fabricfrom a TLV byte-slice, at it allows the TLV byte-slice to be converted to theFabricstruct directly in-place, without first materializing theFabricstruct on-stack, and only then copying it to the finalVecof fabrics.4.
ToTLVextended to support an extra method -to_tlv_iterThis extra method allow the TLV stream to be consumed not by supplying a TLV "writer", but "iterator style", as a sequence of bytes.
Its usage is currently limited and - as its documentation says - it should be used with care, because the resulting iterator - which is really generated as a monadic combination of
map,flat_mapetc. can grow big real quick, thus rendering the whole "iterator based" approach less efficient (w.r.t. memory consumption!) than just using a buffer and serializing in there.5. While at it:
ToTLV::to_tlvgenerified overW: TLVWriteTLVWriteris still there for backwards compatibility, but now the user can plug anything that implementsTLVWriteas the TLV serialization target. Including e.g. aWriteBufwithout wrapping it with aTLVWriter.Scope of changes:
crate::tlvis completely redesigned in that I've tried to split the code into smaller modules, and properly document the complete APIOctetStr = Octets,Utf8Str = &str,ElementType = TLVValue,TagType = TLVTagand so onI also re-desgned the so called
ImEngineby renaming it (and its modules) toE2Eas we can use this module not just for IM tests in future, as it actually is no longer IM-specific.I do realize that there's the ChipTool Python XML E2E tests too (and I'm btw working on getting these to run against
rs-matter) - yet - our E2E tests are much, much faster, as they are essentially executed as in-process unit tests, so I'm not sure these don't have a future.