Replace Write trait with a write fn in Logger.
#258
Conversation
jonas-schievink
added
the
status: blocked
Dirbaio
force-pushed
the
codesize2
branch
2 times, most recently
from
e0f0c9e to
dc01395
Compare
|
triage: added to the 0.2.0 milestone. |
japaric
added
the
breaking change
Dirbaio
force-pushed
the
codesize2
branch
2 times, most recently
from
84930c2 to
f553e86
Compare
|
I was wondering if it's still possible to implement a lock-free logger with the proposed trait changes and it seems possible but these changes may impact performance negatively (don't know by how much though). One way to implement such a logger on Cortex-M is to have one RTT channel per priority level. With this design interrupts running at different priorities don't need to lock (disable interrupts / cause priority inversion) a single logger, instead they grab their interrupt-priority-local (similar to a thread-local) logger. With the current trait, the implementation of that lock-free logger looks like this (off the top of my head: may not be correct / accurate): // an RTT channel
struct Channel { taken: AtomicBool, /* .. */ }
impl defmt::Write for Channel { /* .. */ }
static mut CHANNELS: [Channel; N] = [/* .. */];
unsafe fn grab() -> Option<&mut Channel> {
let static_priority = get_static_priority();
// omitted: scaling priority down to a logical index
let channel = CHANNELS.get_mut(index);
// omitted: check `taken` flag; return `None` if already taken
}
impl defmt::Logger for Logger {
fn acquire() -> Option<NonNull<dyn defmt::Write>> {
let channel = unsafe { acquire() };
// omitted: set `taken` flag; cast into trait object
}
fn release(channel: Option<NonNull<dyn defmt::Write>>) {
// omitted: downcast `channel` to `Channel` then clear `taken`
}
}With the proposed impl defmt::Logger for Logger {
unsafe fn write(bytes: &[u8]) {
// no handle passed as an argument; logger needs to be retrieved again
if let Some(channel) = grab() {
channel.write() // Channel::write
}
}
}So that's extra overhead per |
|
Indexing into Aside from that, this new API allows for inlining I'd be very surprised if it's slower all things considered. |
|
Also, your example can be optimized a bit more: it's enough to check EDIT added code static mut TAKEN: [bool; N] = [/* .. */];
static mut CHANNELS: [UpChannel; N] = [/* .. */];
unsafe fn logical_priority() -> usize {
let static_priority = get_static_priority();
// omitted: scaling priority down to a logical index
let logical_priority = ...
logical_priority
}
impl defmt::Logger for Logger {
fn acquire() -> bool {
let p = logical_priority();
if TAKEN[p] {
return false;
}
TAKEN[p] = true;
true
}
fn release() {
let p = logical_priority();
TAKEN[p] = false;
}
unsafe fn write(bytes: &[u8]) {
let p = logical_priority();
CHANNELS[p].write(bytes);
}
} |
|
Ran the numbers again on my project. This is still 54% code size savings! |
|
Thinking about this, could we make |
|
That would be nearly the same as the current code in |
|
This is now heavily bitrotten. I was hoping this could be in 0.2 but nope :( Is there still interest in doing this? @japaric @jonas-schievink |
|
Yeah, I do think we want this, or something like this. I don't currently have the time to come up with the best solution here though. |
Dirbaio
force-pushed
the
codesize2
branch
2 times, most recently
from
fc39c14 to
143616e
Compare
|
Rebased (rewrote) on latest master. Here are the numbers. This time reduction is only 27%. I still haven't investigated why. Maybe it's other kind of bloat introduced in 0.2 that doesn't get removed by this? For example the #[inline(never)]
fn lol(x: u32) {
info!("defmt test {=u32}", x);
}before: after: |
|
Closing in favor of #505 |
The "remove dyn Write" part of of #257 contained breaking changes, so it's extracted here as a separate PR. See that PR for details.