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

Reduce memory usage by using rref instead of move-copy #6874

Merged
merged 1 commit into from
May 18, 2021

Conversation

kghost
Copy link
Contributor

@kghost kghost commented May 17, 2021

Using rvalue reference can save a call to move constructor.

This PR is trying to replace some arguments of PacketBufferHandle to rvalue reference, in order to check if we can save memory.

Copy link
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Nice result! :)

@woody-apple
Copy link
Contributor

@saurabhst @andy31415 ?

@woody-apple
Copy link
Contributor

@kghost Sadly a conflict now, sorry :(

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 805a4b3

File Section File VM
chip-shell.elf device_handles 12 12
chip-shell.elf text -764 -764
chip-lighting.elf device_handles 8 8
chip-lighting.elf text -1112 -1112
chip-lock.elf device_handles 8 8
chip-lock.elf text -1112 -1112
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
.strtab,0,34
device_handles,12,12
.shstrtab,0,2
.debug_str,0,-3
.debug_aranges,0,-24
.debug_frame,0,-92
.symtab,0,-112
.debug_abbrev,0,-388
text,-764,-764
.debug_ranges,0,-1480
.debug_line,0,-3120
.debug_loc,0,-4649
.debug_info,0,-7556

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

sections,vmsize,filesize
device_handles,8,8
.debug_aranges,0,-56
.debug_str,0,-115
.strtab,0,-116
.debug_frame,0,-120
.symtab,0,-336
.debug_abbrev,0,-489
text,-1112,-1112
.debug_ranges,0,-1208
.debug_line,0,-3956
.debug_loc,0,-4189
.debug_info,0,-9883

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

sections,vmsize,filesize
device_handles,8,8
.debug_aranges,0,-56
.debug_str,0,-115
.strtab,0,-116
.debug_frame,0,-120
.symtab,0,-336
.debug_abbrev,0,-489
text,-1112,-1112
.debug_ranges,0,-1208
.debug_line,0,-3957
.debug_loc,0,-4197
.debug_info,0,-9858


@woody-apple woody-apple merged commit 860a204 into project-chip:master May 18, 2021
@bzbarsky-apple
Copy link
Contributor

@kpschoedel Just FYI here. Given the .text savings I guess we should do it, but this is making a pretty major behavior change to a lot of functions (which used to actually null out the buffer pointer in the caller and now don't, or do but only sometimes). @kghost what auditing did you do to make sure all the callers are OK with that behavior change, and what safeguards to we have in place against testing only situations when the pointer is not nulled out and later then ending up in a case when it is?

@bzbarsky-apple
Copy link
Contributor

testing only situations when the pointer is not nulled out and later then ending up in a case when it is

And to be clear, that is exactly where we are after this PR: a bunch of test shims have the "don't null it out" behavior, while their real-code counterparts would probably null it out.

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.

6 participants