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

Accessor newtypes #465

Merged
merged 4 commits into from
Aug 8, 2020
Merged

Conversation

couchand
Copy link
Contributor

@couchand couchand commented Aug 7, 2020

Part 2 of #463, builds on #464.

  • Wraps register reader/writer and field readers in newtype wrappers, which significantly improves the documentation output.

Again there's some minor potential for breaking changes:

  • If users named the complete type of the previous type alias, it would break. It seems more likely that they would use the alias, which will silently upgrade to a reference to the newtype.

r? @burrbull

@couchand couchand requested a review from a team as a code owner August 7, 2020 04:11
@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Aug 7, 2020
@couchand
Copy link
Contributor Author

couchand commented Aug 7, 2020

bytes master #464 #465
debug 1090816 1094812 1471756
release 7016 7016 105180

Oh my this really blows up the size of the release build! I thought that the newtype pattern was basically optimized away by the compiler, but it seems that either something is preventing that from happening or the compiler's handling of it is not as good as I thought it was.

@couchand
Copy link
Contributor Author

couchand commented Aug 7, 2020

Upon closer inspection, most of that extra junk is in .symtab. After strip --strip-unneeded, the numbers are:

bytes master #464 #465
debug 11148 11148 11772
release 2444 2444 2732

My intuition is that .symtab won't get shipped to flash anyway, so these numbers would be more representative of the real impact. Is that right?

As a general matter, what's the best way to do this sort of profiling?

@couchand
Copy link
Contributor Author

couchand commented Aug 7, 2020

Ok I've stumbled onto size, which seems appropriate for the job. Running that against the binaries here gives me:

debug master #464 #465
text 9386 9386 10010
data 1250 1250 1250
bss 1 1 1
release master #464 #465
text 1862 1862 2150
data 74 74 74
bss 1 1 1

impl core::ops::Deref for R {
type Target = crate::R<#name_uc_spec>;

fn deref(&self) -> &Self::Target {
Copy link
Member

Choose a reason for hiding this comment

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

Add #[inline(always)] for each Deref and DerefMut first and retest.
Do you use cargo size from cargo-binutils or something else?

Copy link
Contributor Author

@couchand couchand Aug 7, 2020

Choose a reason for hiding this comment

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

That did it! Now it's exactly the same size.

These numbers are from my system avr-size. I just tried with cargo-binutils and I'm getting 'Failed to parse crate metadata'.

@couchand
Copy link
Contributor Author

couchand commented Aug 7, 2020

Rebased on top of master, please take another look.


Re: #466 (comment)

For Rahix/avr-device:

$ du --summarize src-*
8884	src-465
7920	src-master

For lpc-rs/lpc-pac:

$ du --summarize lpc*-*
4540	lpc11uxx-465
4168	lpc11uxx-master
7072	lpc176x5x-465
6296	lpc176x5x-master
278760	lpc54606-465
277548	lpc54606-master
4720	lpc82x-465
4212	lpc82x-master
118212	lpc845-465
117632	lpc845-master

There's some increase in the generated code size, but not a lot.

bors bot added a commit that referenced this pull request Aug 7, 2020
466: Avoid register type aliases r=therealprof a=couchand

Part 3 of #463, builds on #465.

- Removes generated use of the register type aliases in favor of directly referencing `Reg<REGISTER_SPEC>`.  I think this helps clarify how the generated and generic types work together.

No breaking change concerns on this change set.

r? @burrbull 

Co-authored-by: Andrew Dona-Couch <hi@andrewcou.ch>
@therealprof
Copy link
Contributor

Size increase looks reasonable. There's still a lot of (medium high hanging) fruit to pick.

I'll soft approve and let give @burrbull have a second look.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit fbf0750 into rust-embedded:master Aug 8, 2020
burrbull added a commit that referenced this pull request Jun 5, 2023
burrbull added a commit that referenced this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants