Support writing FDT from the in-memory representation #8
Conversation
m4tx
force-pushed
the
model
branch
2 times, most recently
from
e283557 to
bb4e565
Compare
m4tx
force-pushed
the
model-writer
branch
2 times, most recently
from
61c7929 to
ddc6cb1
Compare
src/model/writer.rs
Outdated
| const LAST_VERSION: u32 = 17; | ||
| const LAST_COMP_VERSION: u32 = 16; | ||
|
|
||
| pub(crate) fn to_bytes(tree: &DeviceTree) -> Vec<u8> { |
There was a problem hiding this comment.
Having all these methods return Vec<u8> results in a lot a temporary allocations and copying. Instead, take a mutable reference to the place where the bytes should be written. This could either be an &mut [u8] (in which case you probably also want a way to compute ahead of time how big the buffer needs to be), or perhaps &mut impl embedded_io::Write to be more general. The latter is implemented for Vec<u8>.
There was a problem hiding this comment.
We could also just pass &mut Vec<u8> around, right? These are all private functions anyways. Ideally I'd like to avoid adding more dependencies to keep the library lightweight. Computing the exact sizes (which we also need to do for &mut Writer because we need to write the header first) sounds like it could hurt the performance, as this essentially means doing all the writing without actually writing to the buffer (although admittedly it's hard to tell by how much without actually measuring it).
I've changed the code to only allocate one Vec<u8> buffer; let me know if it makes sense.
There was a problem hiding this comment.
This is better than the previous version, but I still don't think it's a great API. It will also still involve multiple allocations, as the backing buffer of the Vec will be reallocated several times as it is expanded.
Separately, why not make this function a method on DeviceTree? If it's just because you want it in this module, you can have another impl block for DeviceTree here.
There was a problem hiding this comment.
Okay, I've modified the code then to preallocate the Vec before writing to it. Note that we still allocate in the StringMap's BTreeMap and when writing the FDT string block, but I don't think we can avoid these. Let me know if that's something you had in mind.
The code is also inside impl DeviceTree block now.
There was a problem hiding this comment.
I'm still not convinced this is the best approach, but let's go ahead and merge this and I'll send another PR later with some ideas.
2 participants
No description provided.