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

Remove reference counting #3356

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

mspang
Copy link
Contributor

@mspang mspang commented Oct 21, 2020

This needs a re-think; there are several problems currently:

  • refcounting of objects with static storage duration
  • refcounting of objects with automatic storage duration (stack objects)
  • refcounting of subobjects
    • including multiple base class subobjects (diamond inheritance)

The majority of current uses are in contexts where lifetime could not be
extended by reference counting and in general, the initial reference is
not dropped. The remaining case looks like a case of unique ownership.
Remove the refcounting since there does not seem to be a case where
it's needed.

If delegate interfaces really need to be refcounted, we'll need to ban
multiple inheritance of them (without resorting to virtual inheritance).

This needs a re-think; there are several problems currently:

- refcounting of objects with static storage duration
- refcounting of objects with automatic storage duration (stack objects)
- refcounting of subobjects
  - including multiple base class subobjects (diamond inheritance)

The majority of current uses are in contexts where lifetime could not be
extended by reference counting and in general, the initial reference is
not dropped. The remaining case looks like a case of unique ownership.

If delegate interfaces really need to be refcounted, we'll need to ban
multiple inheritance of them (without resorting to virtual inheritance).

fixes project-chip#3348
@github-actions
Copy link

Size increase report for "nrfconnect-example-build"

File Section File VM
chip-lock.elf datas -4 -4
chip-lock.elf devices -4 -4
chip-lock.elf bss 0 -13
chip-lock.elf [LOAD #3 [RW]] 0 -19
chip-lock.elf text -448 -448
chip-lighting.elf [LOAD #3 [RW]] 0 27
chip-lighting.elf datas -4 -4
chip-lighting.elf devices -4 -4
chip-lighting.elf bss 0 -27
chip-lighting.elf text -448 -448
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

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

sections,vmsize,filesize
.shstrtab,0,3
datas,-4,-4
devices,-4,-4
bss,-13,0
[LOAD #3 [RW]],-19,0
.debug_aranges,0,-40
.symtab,0,-128
.debug_frame,0,-184
.strtab,0,-379
.debug_abbrev,0,-388
.debug_ranges,0,-432
text,-448,-448
.debug_line,0,-1374
.debug_loc,0,-1417
.debug_str,0,-4271
.debug_info,0,-9494

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

sections,vmsize,filesize
[LOAD #3 [RW]],27,0
.shstrtab,0,-1
datas,-4,-4
devices,-4,-4
bss,-27,0
.debug_aranges,0,-40
.symtab,0,-128
.debug_frame,0,-184
.strtab,0,-379
.debug_abbrev,0,-388
.debug_ranges,0,-432
text,-448,-448
.debug_line,0,-1374
.debug_loc,0,-1425
.debug_str,0,-4271
.debug_info,0,-9494


@github-actions
Copy link

Size increase report for "esp32-example-build"

File Section File VM
chip-wifi-echo.elf .dram0.bss 0 -8
chip-wifi-echo.elf .flash.text -236 -236
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize
.xt.prop._ZN4chip8Encoding12LittleEndian7Write32ERPhj,0,-1
.dram0.bss,-8,0
.debug_aranges,0,-16
.debug_ranges,0,-40
.xt.prop._ZN4chip8BufBound3PutEPKvj,0,-40
.xt.prop._ZN4chip9Transport19PeerConnectionStateC5Ev,0,-40
.debug_frame,0,-48
.xt.prop._ZN4chip16ReferenceCountedINS_28SecurePairingSessionDelegateENS_13DeleteDeletorIS1_EEE7ReleaseEv,0,-88
.xt.prop._ZN4chip16ReferenceCountedINS_9Transport4BaseENS_13DeleteDeletorIS2_EEE7ReleaseEv,0,-88
.symtab,0,-96
.strtab,0,-179
.flash.text,-236,-236
.debug_abbrev,0,-256
.shstrtab,0,-393
.debug_loc,0,-743
.debug_line,0,-1165
.debug_str,0,-2654
.debug_info,0,-4909


@mspang
Copy link
Contributor Author

mspang commented Oct 21, 2020

This is not technically needed to fix #3348, I uploaded a targeted fix for that in #3357.

With that bug fixed, AIUI these refcounts will generally not become zero, so managing them is not really unsafe - just confusing. It's dead code.

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

We really need some saner (auto_ptr-like or unique_ptr-like) reference management. Manually Retain/AddRef/Release/RemoveRef does not see easy to keep track off.

@mspang mspang changed the title Remove refcounting Remove reference counting Oct 22, 2020
@mspang
Copy link
Contributor Author

mspang commented Oct 22, 2020

@mspang mspang merged commit 7c091be into project-chip:master Oct 22, 2020
@mspang mspang deleted the for-chip/remove-refcounting branch October 22, 2020 20:15
mspang added a commit that referenced this pull request Oct 22, 2020
mspang added a commit that referenced this pull request Oct 22, 2020
mspang added a commit to mspang/connectedhomeip that referenced this pull request Oct 22, 2020
This needs a re-think; there are several problems currently:

- refcounting of objects with static storage duration
- refcounting of objects with automatic storage duration (stack objects)
- refcounting of subobjects
  - including multiple base class subobjects (diamond inheritance)

The majority of current uses are in contexts where lifetime could not be
extended by reference counting and in general, the initial reference is
not dropped. The remaining case looks like a case of unique ownership.

If delegate interfaces really need to be refcounted, we'll need to ban
multiple inheritance of them (without resorting to virtual inheritance).

Reland with some #include needed after project-chip#3397.

fixes project-chip#3348
BroderickCarlin pushed a commit that referenced this pull request Oct 26, 2020
* Remove refcounting (reland of #3356)

This needs a re-think; there are several problems currently:

- refcounting of objects with static storage duration
- refcounting of objects with automatic storage duration (stack objects)
- refcounting of subobjects
  - including multiple base class subobjects (diamond inheritance)

The majority of current uses are in contexts where lifetime could not be
extended by reference counting and in general, the initial reference is
not dropped. The remaining case looks like a case of unique ownership.

If delegate interfaces really need to be refcounted, we'll need to ban
multiple inheritance of them (without resorting to virtual inheritance).

Reland with some #include needed after #3397.

fixes #3348

* Add missing #include <CHIPMem.h>

* Add more missing includes
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