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.

Sign up for GitHub

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

Allocation reduction #39

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Allocation reduction #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe2be0e
aml: Add missing test for Scope::raw
aconz2 Aug 16, 2024
5dc9449
misc: Reduce allocations and streamline copies
aconz2 Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 57 additions & 86 deletions src/aml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,7 @@ impl Aml for Path {
};

for part in self.name_parts.clone().iter_mut() {
for byte in part {
aconz2 marked this conversation as resolved.
Show resolved Hide resolved
sink.byte(*byte);
}
sink.vec(part);
}
}
}
Expand Down Expand Up @@ -233,9 +231,7 @@ pub struct Name {

impl Aml for Name {
fn to_aml_bytes(&self, sink: &mut dyn AmlSink) {
for byte in &self.bytes {
sink.byte(*byte);
}
sink.vec(&self.bytes);
}
}

Expand Down Expand Up @@ -273,13 +269,10 @@ impl<'a> Aml for Package<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would agree that reversing and inserting at 0 is usually equivalent to appending. But I think @timw-rivos must have had a reason for doing it this way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I just thought it through once and didn't look at it again ;)

pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(PACKAGEOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -312,6 +305,10 @@ impl AmlSink for PackageBuilder {
fn byte(&mut self, byte: u8) {
self.data.push(byte);
}

fn vec(&mut self, v: &[u8]) {
self.data.extend_from_slice(v);
}
}

impl PackageBuilder {
Expand Down Expand Up @@ -345,13 +342,10 @@ impl<'a> Aml for VarPackageTerm<'a> {
let mut bytes = Vec::new();
self.data.to_aml_bytes(&mut bytes);

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(VARPACKAGEOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -380,7 +374,7 @@ package length is 2**28."

/* Also used for NamedField but in that case the length is not included in itself */
fn create_pkg_length(len: usize, include_self: bool) -> Vec<u8> {
let mut result = Vec::new();
let mut result = Vec::with_capacity(4);

/* PkgLength is inclusive and includes the length bytes */
let length_length = if len < (2usize.pow(6) - 1) {
Expand Down Expand Up @@ -466,9 +460,7 @@ impl Aml for Usize {

fn create_aml_string(v: &str, sink: &mut dyn AmlSink) {
sink.byte(STRINGOP);
for byte in v.as_bytes() {
sink.byte(*byte);
}
sink.vec(v.as_bytes());
sink.byte(0x0); /* NullChar */
}

Expand Down Expand Up @@ -510,21 +502,15 @@ impl<'a> Aml for ResourceTemplate<'a> {

// Buffer length is an encoded integer including buffer data
// and EndTag and checksum byte
let mut buffer_length = Vec::new();
let mut buffer_length = Vec::with_capacity(4);
bytes.len().to_aml_bytes(&mut buffer_length);
buffer_length.reverse();
for byte in buffer_length {
bytes.insert(0, byte);
}

// PkgLength is everything else
let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len() + buffer_length.len(), true);

sink.byte(BUFFEROP);
sink.vec(&pkg_length);
sink.vec(&buffer_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -806,14 +792,11 @@ impl<'a> Aml for Device<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(EXTOPPREFIX); /* ExtOpPrefix */
sink.byte(DEVICEOP); /* DeviceOp */
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand All @@ -839,13 +822,10 @@ impl<'a> Aml for Scope<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(SCOPEOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand All @@ -859,14 +839,19 @@ impl<'a> Scope<'a> {
/// Create raw bytes representing a Scope from its children in raw bytes
pub fn raw(path: Path, mut children: Vec<u8>) -> Vec<u8> {
let mut bytes = Vec::new();
bytes.push(SCOPEOP);
path.to_aml_bytes(&mut bytes);
bytes.append(&mut children);
let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
bytes.insert(0, SCOPEOP);

let n = bytes.len(); // n >= 1
let pkg_length = create_pkg_length(n - 1, true);
let m = pkg_length.len();

// move everything after the SCOPEOP over and copy in pkg_length
bytes.resize(n + m, 0xFF);
bytes.as_mut_slice().copy_within(1..n, m + 1);
bytes.as_mut_slice()[1..m + 1].copy_from_slice(pkg_length.as_slice());

bytes
}
}
Expand Down Expand Up @@ -901,13 +886,10 @@ impl<'a> Aml for Method<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(METHODOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -996,14 +978,11 @@ impl Aml for Field {
}
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(EXTOPPREFIX);
sink.byte(FIELDOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -1078,13 +1057,10 @@ impl<'a> Aml for If<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(IFOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand All @@ -1108,13 +1084,10 @@ impl<'a> Aml for Else<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(ELSEOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -1310,13 +1283,10 @@ impl<'a> Aml for While<'a> {
child.to_aml_bytes(&mut bytes);
}

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(WHILEOP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -1530,13 +1500,10 @@ impl<'a> Aml for BufferTerm<'a> {
let mut bytes = Vec::new();
self.data.to_aml_bytes(&mut bytes);

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(BUFFEROP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand All @@ -1559,13 +1526,10 @@ impl Aml for BufferData {
self.data.len().to_aml_bytes(&mut bytes);
bytes.extend_from_slice(&self.data);

let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(BUFFEROP);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -1683,14 +1647,11 @@ impl<'a> Aml for PowerResource<'a> {
}

// PkgLength
let mut pkg_length = create_pkg_length(bytes.len(), true);
pkg_length.reverse();
for byte in pkg_length {
bytes.insert(0, byte);
}
let pkg_length = create_pkg_length(bytes.len(), true);

sink.byte(POWERRESOURCEOP);
sink.byte(EXTOPPREFIX);
sink.vec(&pkg_length);
sink.vec(&bytes);
}
}
Expand Down Expand Up @@ -1779,6 +1740,16 @@ mod tests {
assert_eq!(aml, &mbrd_scope[..]);
}

aconz2 marked this conversation as resolved.
Show resolved Hide resolved
#[test]
fn test_scope_raw() {
let scope = [
0x10, 0x0E, 0x2E, 0x5F, 0x53, 0x42, 0x5F, 0x4D, 0x42, 0x52, 0x44, 0xAA, 0xBB, 0xCC,
0xDD,
];
let bytes = Scope::raw("_SB_.MBRD".into(), vec![0xAA, 0xBB, 0xCC, 0xDD]);
assert_eq!(bytes, scope);
}

#[test]
fn test_resource_template() {
/*
Expand Down
4 changes: 1 addition & 3 deletions src/cedt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,7 @@ impl Aml for CxlFixedMemory {
sink.word(self.window_restrictions);
sink.word(self.qtg_id);
for target in &self.interleave_targets {
for byte in target {
sink.byte(*byte);
}
sink.vec(target);
}
}
}
Expand Down
4 changes: 1 addition & 3 deletions src/fadt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,7 @@ impl FADT {

impl Aml for FADT {
fn to_aml_bytes(&self, sink: &mut dyn AmlSink) {
for byte in self.table.as_bytes() {
sink.byte(*byte);
}
sink.vec(self.table.as_bytes());
}
}

Expand Down
20 changes: 8 additions & 12 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,15 @@ pub trait AmlSink {
fn byte(&mut self, byte: u8);

fn word(&mut self, word: u16) {
for byte in word.to_le_bytes() {
self.byte(byte);
}
self.vec(&word.to_le_bytes());
}

fn dword(&mut self, dword: u32) {
for byte in dword.to_le_bytes() {
self.byte(byte);
}
self.vec(&dword.to_le_bytes());
}

fn qword(&mut self, qword: u64) {
for byte in qword.to_le_bytes() {
self.byte(byte);
}
self.vec(&qword.to_le_bytes());
}

fn vec(&mut self, v: &[u8]) {
Expand All @@ -87,6 +81,10 @@ impl AmlSink for alloc::vec::Vec<u8> {
fn byte(&mut self, byte: u8) {
self.push(byte);
}

fn vec(&mut self, v: &[u8]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should rename this API to e.g.

AmlSink::sink_{byte, word, dword, qword, slice} @sboeuf @timw-rivos WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is private to this library, but it is more clear

self.extend_from_slice(v);
}
}

/// Standard header for many ACPI tables
Expand Down Expand Up @@ -187,9 +185,7 @@ macro_rules! aml_as_bytes {
($x:ty) => {
impl Aml for $x {
fn to_aml_bytes(&self, sink: &mut dyn AmlSink) {
for byte in self.as_bytes() {
sink.byte(*byte);
}
sink.vec(self.as_bytes());
}
}
};
Expand Down
4 changes: 1 addition & 3 deletions src/madt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,7 @@ impl MADT {

impl Aml for MADT {
fn to_aml_bytes(&self, sink: &mut dyn AmlSink) {
for byte in self.header.as_bytes() {
sink.byte(*byte);
}
sink.vec(self.header.as_bytes());

for st in &self.structures {
st.to_aml_bytes(sink);
Expand Down
Loading
Loading