FluenTech / embedded-time

Time(ing) library (Instant/Duration/Clock/Timer/Period/Frequency) for bare-metal embedded systems
Apache License 2.0
87 stars 17 forks source link

Code bloat due to use of `unwrap` in library #84

Closed korken89 closed 3 years ago

korken89 commented 3 years ago

Hi, I did some code bloat tests and as I suspected the use of unwrap() in the library does pull in unnecessary formatting machinery. I can fix it but I want to check first, all val.unwrap() would be replaced with:

if let Some(v) = val { // Or `Ok(v)` as needed
    v
} else {
    panic!("Operation X failed.")
}

Should I fix a PR with this?

TDHolmes commented 3 years ago

Are there numbers on how much bloat this causes?

korken89 commented 3 years ago

Not yet sure, as I converted my entire codebase to embedded time before realising, but I can run a cargo bloat and see what was added.

PTaylor-us commented 3 years ago

If I remember correctly, I tested for bloat pretty extensively and while the .unwrap()s I used does add some code, it was negligible and not worth making the code quite so noisy.

korken89 commented 3 years ago

Hi, here are the most prominent ones that popped up when adding embedded-time to our project and replacing all time usage with it:

0.0%   1.5%    874B                 std core::fmt::Formatter::pad
0.0%   1.0%    604B                 std core::fmt::num::<impl core::fmt::Debug for u64>::fmt
0.0%   0.8%    484B                 std core::fmt::write

I am not 100% sure if the first line is attributed to embedded-time though, as it's errors should not need padding. But I am not sure.

korken89 commented 3 years ago

@PTaylor-us On the "noisy" part, simply create a macro to reduce it.

As I stated before, I see no issue in me implementing this.

korken89 commented 3 years ago

We are now encountering serious bloat issues which leads me back to this, see the following issue https://github.com/rtic-rs/cortex-m-rtic/issues/472

PTaylor-us commented 3 years ago

@korken89 Was it verified as to what was pulling in those formatting routines? I see a lot of float formatting routines. I'm not sure how embedded_time could be causing those dependencies. I'm not averse to eliminating any fmt dependencies using a macro as you suggest. I realize that some projects will have to count bytes of code size.

korken89 commented 3 years ago

To get to the full bottom of this I will make a branch of embedded-time to remove all use of panic with Debug and see where we land. Just adding/removing dependencies points to embedded-time as the common denominator, but I want to be 100% sure from where this comes with some reproducible examples as well.

It's quite unfortunate that it's generally so hard to track formating bloat down.