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

Begin SystemPacketBuffer.h cleanup #4096

Merged
merged 5 commits into from
Dec 8, 2020

Conversation

kpschoedel
Copy link
Contributor

Problem

Code should use PacketBufferHandle rather than PacketBuffer *.
Some existing methods should be removed or made private so that code
does not have unnecessary access to the raw pointer.

Summary of Changes

  • Consolidated PacketBuffer and PacketBufferHeader method descriptions
    into the header, and clarified which are in transition.
  • Moved trivial PacketBuffer getters inline.
  • Most uses of Next(), which returns a raw pointer, only checked
    for existence; added HasChainedBuffer() to replace these.
  • Removed DetachTail_ForNow(), as it has no remaining callers.
  • Converted TestSystemPacketBuffer.cpp to use a friend class, so that
    it can continue to use and test private methods. (Refactoring to focus
    on the PacketBufferHandle interface will follow.)

Part of issue #2707 - Figure out a way to express PacketBuffer ownership in the type system

#### Problem

Code should use `PacketBufferHandle` rather than `PacketBuffer *`.
Some existing methods should be removed or made private so that code
does not have unnecessary access to the raw pointer.

#### Summary of Changes

- Consolidated PacketBuffer and PacketBufferHeader method descriptions
  into the header; clarified which are in transition.
- Most uses of Next(), which returns a raw pointer, only checked
  existence; added PacketBuffer::HasChainedBuffer() to replace these.
- Removed DetachTail_ForNow(), as it has no remaining callers.
- Converted TestSystemPacketBuffer to friended class, so that it can
  continue to use and test private methods. (Refactoring to focus on the
  PacketBufferHandle interface will follow.)

Part of issue project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system
@github-actions
Copy link

github-actions bot commented Dec 4, 2020

Size increase report for "nrfconnect-example-build" from 2244db6

File Section File VM
chip-lighting.elf rodata -40 -40
chip-lighting.elf text -560 -560
chip-shell.elf text -176 -176
chip-lock.elf rodata -40 -40
chip-lock.elf text -560 -560
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,8429
.debug_abbrev,0,2699
.debug_line,0,480
.debug_str,0,279
.shstrtab,0,-1
.debug_aranges,0,-40
rodata,-40,-40
.symtab,0,-96
.strtab,0,-123
.debug_frame,0,-140
.debug_ranges,0,-184
.debug_loc,0,-355
text,-560,-560

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

sections,vmsize,filesize
.debug_info,0,4847
.debug_abbrev,0,1416
.debug_str,0,279
.shstrtab,0,-3
.debug_aranges,0,-40
.symtab,0,-64
.strtab,0,-77
.debug_line,0,-95
.debug_frame,0,-112
.debug_ranges,0,-128
text,-176,-176
.debug_loc,0,-439

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

sections,vmsize,filesize
.debug_info,0,8606
.debug_abbrev,0,2745
.debug_line,0,477
.debug_str,0,279
.shstrtab,0,3
.debug_aranges,0,-40
rodata,-40,-40
.symtab,0,-96
.strtab,0,-123
.debug_frame,0,-140
.debug_ranges,0,-184
.debug_loc,0,-351
text,-560,-560


@github-actions
Copy link

github-actions bot commented Dec 4, 2020

Size increase report for "esp32-example-build" from 2244db6

File Section File VM
chip-all-clusters-app.elf .flash.rodata -40 -40
chip-all-clusters-app.elf .flash.text -432 -432
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
.debug_info,0,11270
.debug_abbrev,0,1349
.debug_line,0,992
.debug_loc,0,629
.debug_str,0,342
.xt.prop._ZN4chip6System12PacketBuffer13SetDataLengthEt,0,116
.shstrtab,0,111
.debug_ranges,0,104
[Unmapped],0,40
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,-2
.symtab,0,-16
.debug_aranges,0,-40
.flash.rodata,-40,-40
.debug_frame,0,-120
.strtab,0,-123
.flash.text,-432,-432


* Return the size of the allocation including the reserved and payload data spaces but not including space
* allocated for the PacketBuffer structure.
*
* @note The allocation size is equal or greater than \c aAllocSize paramater to \c Create method).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @note The allocation size is equal or greater than \c aAllocSize paramater to \c Create method).
* @note The allocation size is equal to or greater than the \c aAllocSize parameter to the \c Create method).

* @brief The caller's ownership is transferred to this. An existing owned buffer is freed.
*
* @note This should only be used in low-level code, e.g. to import buffers from LwIP or a similar stack.
*/
void Adopt(PacketBuffer * buffer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking of purging this, even internal to PacketBufferHandle, because I've more than once made errors due to the asymmetry: it Frees the old mBuffer but does not AddRef the new one.

* Return the size of the allocation including the reserved and payload data spaces but not including space
* allocated for the PacketBuffer structure.
*
* @note The allocation size is equal or to greater than \c aAllocSize parameter to the \c Create method).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @note The allocation size is equal or to greater than \c aAllocSize parameter to the \c Create method).
* @note The allocation size is equal to or greater than the \c aAllocSize parameter to the \c Create method).

@rwalker-apple rwalker-apple merged commit 548b71a into project-chip:master Dec 8, 2020
@kpschoedel kpschoedel deleted the x2707-12-header branch December 8, 2020 18:16
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.

5 participants