-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Code size optimizations #257
Conversation
d5f003e
to
5e30729
Compare
Really nice! We might want to hold off on the breaking changes though, since we probably want to wait a bit until releasing 0.2.0, so if you can split them into a separate PR that'd be great! |
Done! |
@@ -421,10 +421,8 @@ fn log(level: Level, ts: TokenStream) -> TokenStream { | |||
if #logging_enabled { | |||
match (#(&(#args)),*) { | |||
(#(#pats),*) => { | |||
let ts = defmt::export::timestamp(); | |||
if let Some(mut _fmt_) = defmt::export::acquire() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also make the acquire
part of shared code, by doing something like:
impl Formatter {
pub fn start_frame(s: &Str) -> Option<Self> {
let ts = timestamp();
let mut fmt = acquire();
if let Some(fmt) = &mut fmt {
fmt.istr(s);
fmt.leb64(ts);
}
fmt
}
}
As a bonus, this keeps the order of operations like it was before (timestamp, then acquire).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably we'd want to move the #[inline(never)]
to start_frame
, since we should only have a single call site to acquire
remaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea. What do we do with winfo!
then? It needs to encode the header but doesn't create its own Formatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, good question. Ideally I'd like to get rid of it. I might look into doing that tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge this PR as-is for now – we can always land further improvements later.
dyn Write
with an extraWrite
function in Logger trait. This saves quite a bit of space because now many Formatter methods no longer require access toself
, which allows nice optimizations. -- MOVED from this PR into Replace Write trait with awrite
fn in Logger. #258timestamp
call, then write the log string withistr
, then write the timestamp withleb64
. Extract this to a singleheader
function. Replaces 3 calls with 1, though most of the savings here come from#[inline(never)]
because the compiler was inliningtimestamp
.#[inline(never)]
toacquire
andrelease
because the compiler was inlining them.With these changes, defmt code size goes from 62kb to 16kb In my project's firmware. This is a 73,8% code size reduction.
Generated code for this function:
Before:
After: