From eef91929871996722fae120e2ddc57abf651d71a Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Mon, 29 Jan 2024 19:48:05 +0000 Subject: [PATCH] aconfig: remove bucket_index from PackageTableNode/FlagTableNode struct bucket index currently is a field in PackageTableNode/FlagTableNode, but this is purely aux info that is never searilized or deserialized. Therefore we should remove it from the struct definition. Instead aconfig should define a wrapper struct that wraps around an instance PackageTableNode/FlagTableNode as well as aux info like bucket_index. Bug: 321077378 Test: atest aconfig.test && atest aconfig_storage_file.test Change-Id: I20f2565d20b7feb5d39754e91cd6a9affb1f0e70 --- .../aconfig/aconfig/src/storage/flag_table.rs | 148 ++++++++++-------- .../aconfig/src/storage/package_table.rs | 75 +++++---- .../aconfig_storage_file/src/flag_table.rs | 36 ++--- .../aconfig_storage_file/src/package_table.rs | 26 ++- 4 files changed, 152 insertions(+), 133 deletions(-) diff --git a/tools/aconfig/aconfig/src/storage/flag_table.rs b/tools/aconfig/aconfig/src/storage/flag_table.rs index bebac890a0..4dd177c6a3 100644 --- a/tools/aconfig/aconfig/src/storage/flag_table.rs +++ b/tools/aconfig/aconfig/src/storage/flag_table.rs @@ -32,40 +32,58 @@ fn new_header(container: &str, num_flags: u32) -> FlagTableHeader { } } -fn new_node( - package_id: u32, - flag_name: &str, - flag_type: u16, - flag_id: u16, - num_buckets: u32, -) -> FlagTableNode { - let bucket_index = FlagTableNode::find_bucket_index(package_id, flag_name, num_buckets); - FlagTableNode { - package_id, - flag_name: flag_name.to_string(), - flag_type, - flag_id, - next_offset: None, - bucket_index, - } +// a struct that contains FlagTableNode and a bunch of other information to help +// flag table creation +#[derive(PartialEq, Debug, Clone)] +struct FlagTableNodeWrapper { + pub node: FlagTableNode, + pub bucket_index: u32, } -fn create_nodes(package: &FlagPackage, num_buckets: u32) -> Result> { - let flag_ids = assign_flag_ids(package.package_name, package.boolean_flags.iter().copied())?; - package - .boolean_flags - .iter() - .map(|&pf| { - let fid = flag_ids - .get(pf.name()) - .ok_or(anyhow!(format!("missing flag id for {}", pf.name())))?; - // all flags are boolean value at the moment, thus using the last bit. When more - // flag value types are supported, flag value type information should come from the - // parsed flag, and we will set the flag_type bit mask properly. - let flag_type = 1; - Ok(new_node(package.package_id, pf.name(), flag_type, *fid, num_buckets)) - }) - .collect::>>() +impl FlagTableNodeWrapper { + fn new( + package_id: u32, + flag_name: &str, + flag_type: u16, + flag_id: u16, + num_buckets: u32, + ) -> Self { + let bucket_index = FlagTableNode::find_bucket_index(package_id, flag_name, num_buckets); + let node = FlagTableNode { + package_id, + flag_name: flag_name.to_string(), + flag_type, + flag_id, + next_offset: None, + }; + Self { node, bucket_index } + } + + fn create_nodes(package: &FlagPackage, num_buckets: u32) -> Result> { + let flag_ids = + assign_flag_ids(package.package_name, package.boolean_flags.iter().copied())?; + package + .boolean_flags + .iter() + .map(|&pf| { + let fid = flag_ids + .get(pf.name()) + .ok_or(anyhow!(format!("missing flag id for {}", pf.name())))?; + // all flags are boolean value at the moment, thus using the last bit. + // When more flag value types are supported, flag value type information + // should come from the parsed flag, and we will set the flag_type bit + // mask properly. + let flag_type = 1; + Ok(Self::new( + package.package_id, + pf.name(), + flag_type, + *fid, + num_buckets, + )) + }) + .collect::>>() + } } pub fn create_flag_table(container: &str, packages: &[FlagPackage]) -> Result { @@ -73,44 +91,48 @@ pub fn create_flag_table(container: &str, packages: &[FlagPackage]) -> Result>>()? - .concat(), - }; + let mut header = new_header(container, num_flags); + let mut buckets = vec![None; num_buckets as usize]; + let mut node_wrappers = packages + .iter() + .map(|pkg| FlagTableNodeWrapper::create_nodes(pkg, num_buckets)) + .collect::>>()? + .concat(); // initialize all header fields - table.header.bucket_offset = table.header.as_bytes().len() as u32; - table.header.node_offset = table.header.bucket_offset + num_buckets * 4; - table.header.file_size = table.header.node_offset - + table.nodes.iter().map(|x| x.as_bytes().len()).sum::() as u32; + header.bucket_offset = header.as_bytes().len() as u32; + header.node_offset = header.bucket_offset + num_buckets * 4; + header.file_size = header.node_offset + + node_wrappers.iter().map(|x| x.node.as_bytes().len()).sum::() as u32; // sort nodes by bucket index for efficiency - table.nodes.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index)); + node_wrappers.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index)); // fill all node offset - let mut offset = table.header.node_offset; - for i in 0..table.nodes.len() { - let node_bucket_idx = table.nodes[i].bucket_index; + let mut offset = header.node_offset; + for i in 0..node_wrappers.len() { + let node_bucket_idx = node_wrappers[i].bucket_index; let next_node_bucket_idx = - if i + 1 < table.nodes.len() { Some(table.nodes[i + 1].bucket_index) } else { None }; + if i + 1 < node_wrappers.len() { Some(node_wrappers[i + 1].bucket_index) } else { None }; - if table.buckets[node_bucket_idx as usize].is_none() { - table.buckets[node_bucket_idx as usize] = Some(offset); + if buckets[node_bucket_idx as usize].is_none() { + buckets[node_bucket_idx as usize] = Some(offset); } - offset += table.nodes[i].as_bytes().len() as u32; + offset += node_wrappers[i].node.as_bytes().len() as u32; if let Some(index) = next_node_bucket_idx { if index == node_bucket_idx { - table.nodes[i].next_offset = Some(offset); + node_wrappers[i].node.next_offset = Some(offset); } } } + let table = FlagTable { + header, + buckets, + nodes: node_wrappers.into_iter().map(|nw| nw.node).collect(), + }; + Ok(table) } @@ -126,7 +148,6 @@ mod tests { flag_type: u16, flag_id: u16, next_offset: Option, - bucket_index: u32, ) -> FlagTableNode { FlagTableNode { package_id, @@ -134,7 +155,6 @@ mod tests { flag_type, flag_id, next_offset, - bucket_index, } } @@ -186,13 +206,13 @@ mod tests { let nodes: &Vec = &flag_table.as_ref().unwrap().nodes; assert_eq!(nodes.len(), 8); - assert_eq!(nodes[0], new_expected_node(0, "enabled_ro", 1, 1, None, 0)); - assert_eq!(nodes[1], new_expected_node(0, "enabled_rw", 1, 2, Some(150), 1)); - assert_eq!(nodes[2], new_expected_node(1, "disabled_ro", 1, 0, None, 1)); - assert_eq!(nodes[3], new_expected_node(2, "enabled_ro", 1, 1, None, 5)); - assert_eq!(nodes[4], new_expected_node(1, "enabled_fixed_ro", 1, 1, Some(235), 7)); - assert_eq!(nodes[5], new_expected_node(1, "enabled_ro", 1, 2, None, 7)); - assert_eq!(nodes[6], new_expected_node(2, "enabled_fixed_ro", 1, 0, None, 9)); - assert_eq!(nodes[7], new_expected_node(0, "disabled_rw", 1, 0, None, 15)); + assert_eq!(nodes[0], new_expected_node(0, "enabled_ro", 1, 1, None)); + assert_eq!(nodes[1], new_expected_node(0, "enabled_rw", 1, 2, Some(150))); + assert_eq!(nodes[2], new_expected_node(1, "disabled_ro", 1, 0, None)); + assert_eq!(nodes[3], new_expected_node(2, "enabled_ro", 1, 1, None)); + assert_eq!(nodes[4], new_expected_node(1, "enabled_fixed_ro", 1, 1, Some(235))); + assert_eq!(nodes[5], new_expected_node(1, "enabled_ro", 1, 2, None)); + assert_eq!(nodes[6], new_expected_node(2, "enabled_fixed_ro", 1, 0, None)); + assert_eq!(nodes[7], new_expected_node(0, "disabled_rw", 1, 0, None)); } } diff --git a/tools/aconfig/aconfig/src/storage/package_table.rs b/tools/aconfig/aconfig/src/storage/package_table.rs index f82e932b0e..5ce6165ab8 100644 --- a/tools/aconfig/aconfig/src/storage/package_table.rs +++ b/tools/aconfig/aconfig/src/storage/package_table.rs @@ -17,7 +17,7 @@ use anyhow::Result; use aconfig_storage_file::{ - get_bucket_index, get_table_size, PackageTable, PackageTableHeader, PackageTableNode, + get_table_size, PackageTable, PackageTableHeader, PackageTableNode, FILE_VERSION, }; @@ -34,14 +34,24 @@ fn new_header(container: &str, num_packages: u32) -> PackageTableHeader { } } -fn new_node(package: &FlagPackage, num_buckets: u32) -> PackageTableNode { - let bucket_index = get_bucket_index(&package.package_name.to_string(), num_buckets); - PackageTableNode { - package_name: String::from(package.package_name), - package_id: package.package_id, - boolean_offset: package.boolean_offset, - next_offset: None, - bucket_index, +// a struct that contains PackageTableNode and a bunch of other information to help +// package table creation +#[derive(PartialEq, Debug)] +struct PackageTableNodeWrapper { + pub node: PackageTableNode, + pub bucket_index: u32, +} + +impl PackageTableNodeWrapper { + fn new(package: &FlagPackage, num_buckets: u32) -> Self { + let node = PackageTableNode { + package_name: String::from(package.package_name), + package_id: package.package_id, + boolean_offset: package.boolean_offset, + next_offset: None, + }; + let bucket_index = PackageTableNode::find_bucket_index(package.package_name, num_buckets); + Self { node, bucket_index } } } @@ -49,40 +59,46 @@ pub fn create_package_table(container: &str, packages: &[FlagPackage]) -> Result // create table let num_packages = packages.len() as u32; let num_buckets = get_table_size(num_packages)?; - let mut table = PackageTable { - header: new_header(container, num_packages), - buckets: vec![None; num_buckets as usize], - nodes: packages.iter().map(|pkg| new_node(pkg, num_buckets)).collect(), - }; + let mut header = new_header(container, num_packages); + let mut buckets = vec![None; num_buckets as usize]; + let mut node_wrappers: Vec<_> = packages + .iter() + .map(|pkg| PackageTableNodeWrapper::new(pkg, num_buckets)) + .collect(); // initialize all header fields - table.header.bucket_offset = table.header.as_bytes().len() as u32; - table.header.node_offset = table.header.bucket_offset + num_buckets * 4; - table.header.file_size = table.header.node_offset - + table.nodes.iter().map(|x| x.as_bytes().len()).sum::() as u32; + header.bucket_offset = header.as_bytes().len() as u32; + header.node_offset = header.bucket_offset + num_buckets * 4; + header.file_size = header.node_offset + + node_wrappers.iter().map(|x| x.node.as_bytes().len()).sum::() as u32; - // sort nodes by bucket index for efficiency - table.nodes.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index)); + // sort node_wrappers by bucket index for efficiency + node_wrappers.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index)); // fill all node offset - let mut offset = table.header.node_offset; - for i in 0..table.nodes.len() { - let node_bucket_idx = table.nodes[i].bucket_index; + let mut offset = header.node_offset; + for i in 0..node_wrappers.len() { + let node_bucket_idx = node_wrappers[i].bucket_index; let next_node_bucket_idx = - if i + 1 < table.nodes.len() { Some(table.nodes[i + 1].bucket_index) } else { None }; + if i + 1 < node_wrappers.len() { Some(node_wrappers[i + 1].bucket_index) } else { None }; - if table.buckets[node_bucket_idx as usize].is_none() { - table.buckets[node_bucket_idx as usize] = Some(offset); + if buckets[node_bucket_idx as usize].is_none() { + buckets[node_bucket_idx as usize] = Some(offset); } - offset += table.nodes[i].as_bytes().len() as u32; + offset += node_wrappers[i].node.as_bytes().len() as u32; if let Some(index) = next_node_bucket_idx { if index == node_bucket_idx { - table.nodes[i].next_offset = Some(offset); + node_wrappers[i].node.next_offset = Some(offset); } } } + let table = PackageTable { + header, + buckets, + nodes: node_wrappers.into_iter().map(|nw| nw.node).collect(), + }; Ok(table) } @@ -125,7 +141,6 @@ mod tests { package_id: 1, boolean_offset: 3, next_offset: None, - bucket_index: 0, }; assert_eq!(nodes[0], first_node_expected); let second_node_expected = PackageTableNode { @@ -133,7 +148,6 @@ mod tests { package_id: 0, boolean_offset: 0, next_offset: Some(158), - bucket_index: 3, }; assert_eq!(nodes[1], second_node_expected); let third_node_expected = PackageTableNode { @@ -141,7 +155,6 @@ mod tests { package_id: 2, boolean_offset: 6, next_offset: None, - bucket_index: 3, }; assert_eq!(nodes[2], third_node_expected); } diff --git a/tools/aconfig/aconfig_storage_file/src/flag_table.rs b/tools/aconfig/aconfig_storage_file/src/flag_table.rs index 421f84705d..99d5a60ff3 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_table.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_table.rs @@ -68,7 +68,6 @@ pub struct FlagTableNode { pub flag_type: u16, pub flag_id: u16, pub next_offset: Option, - pub bucket_index: u32, } impl FlagTableNode { @@ -97,7 +96,6 @@ impl FlagTableNode { 0 => None, val => Some(val), }, - bucket_index: 0, }; Ok(node) } @@ -142,13 +140,11 @@ impl FlagTable { .collect(); let nodes = (0..num_flags) .map(|_| { - let mut node = FlagTableNode::from_bytes(&bytes[head..]).unwrap(); + let node = FlagTableNode::from_bytes(&bytes[head..])?; head += node.as_bytes().len(); - node.bucket_index = FlagTableNode::find_bucket_index( - node.package_id, &node.flag_name, num_buckets); - node + Ok(node) }) - .collect(); + .collect::>>()?; let table = Self { header, buckets, nodes }; Ok(table) @@ -203,7 +199,6 @@ mod tests { flag_type: u16, flag_id: u16, next_offset: Option, - bucket_index: u32, ) -> Self { Self { package_id, @@ -211,7 +206,6 @@ mod tests { flag_type, flag_id, next_offset, - bucket_index, } } } @@ -245,14 +239,14 @@ mod tests { None, ]; let nodes = vec![ - FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None, 0), - FlagTableNode::new_expected(0, "enabled_rw", 1, 2, Some(150), 1), - FlagTableNode::new_expected(1, "disabled_ro", 1, 0, None, 1), - FlagTableNode::new_expected(2, "enabled_ro", 1, 1, None, 5), - FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, 1, Some(235), 7), - FlagTableNode::new_expected(1, "enabled_ro", 1, 2, None, 7), - FlagTableNode::new_expected(2, "enabled_fixed_ro", 1, 0, None, 9), - FlagTableNode::new_expected(0, "disabled_rw", 1, 0, None, 15), + FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None), + FlagTableNode::new_expected(0, "enabled_rw", 1, 2, Some(150)), + FlagTableNode::new_expected(1, "disabled_ro", 1, 0, None), + FlagTableNode::new_expected(2, "enabled_ro", 1, 1, None), + FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, 1, Some(235)), + FlagTableNode::new_expected(1, "enabled_ro", 1, 2, None), + FlagTableNode::new_expected(2, "enabled_fixed_ro", 1, 0, None), + FlagTableNode::new_expected(0, "disabled_rw", 1, 0, None), ]; Ok(FlagTable { header, buckets, nodes }) } @@ -268,14 +262,8 @@ mod tests { assert_eq!(header, &reinterpreted_header.unwrap()); let nodes: &Vec = &flag_table.nodes; - let num_buckets = crate::get_table_size(header.num_flags).unwrap(); for node in nodes.iter() { - let mut reinterpreted_node = FlagTableNode::from_bytes(&node.as_bytes()).unwrap(); - reinterpreted_node.bucket_index = FlagTableNode::find_bucket_index( - reinterpreted_node.package_id, - &reinterpreted_node.flag_name, - num_buckets - ); + let reinterpreted_node = FlagTableNode::from_bytes(&node.as_bytes()).unwrap(); assert_eq!(node, &reinterpreted_node); } diff --git a/tools/aconfig/aconfig_storage_file/src/package_table.rs b/tools/aconfig/aconfig_storage_file/src/package_table.rs index 8fd57e6061..a3ad6ec5b0 100644 --- a/tools/aconfig/aconfig_storage_file/src/package_table.rs +++ b/tools/aconfig/aconfig_storage_file/src/package_table.rs @@ -69,7 +69,6 @@ pub struct PackageTableNode { // boolean flag value array in the flag value file pub boolean_offset: u32, pub next_offset: Option, - pub bucket_index: u32, } impl PackageTableNode { @@ -96,10 +95,16 @@ impl PackageTableNode { 0 => None, val => Some(val), }, - bucket_index: 0, }; Ok(node) } + + /// Get the bucket index for a package table node, defined it here so the + /// construction side (aconfig binary) and consumption side (flag read lib) + /// use the same method of hashing + pub fn find_bucket_index(package: &str, num_buckets: u32) -> u32 { + get_bucket_index(&package, num_buckets) + } } /// Package table struct @@ -135,12 +140,11 @@ impl PackageTable { .collect(); let nodes = (0..num_packages) .map(|_| { - let mut node = PackageTableNode::from_bytes(&bytes[head..]).unwrap(); + let node = PackageTableNode::from_bytes(&bytes[head..])?; head += node.as_bytes().len(); - node.bucket_index = get_bucket_index(&node.package_name, num_buckets); - node + Ok(node) }) - .collect(); + .collect::>>()?; let table = Self { header, buckets, nodes }; Ok(table) @@ -166,7 +170,7 @@ pub fn find_package_offset(buf: &[u8], package: &str) -> Result = &package_table.nodes; - let num_buckets = crate::get_table_size(header.num_packages).unwrap(); for node in nodes.iter() { - let mut reinterpreted_node = PackageTableNode::from_bytes(&node.as_bytes()).unwrap(); - reinterpreted_node.bucket_index = - get_bucket_index(&reinterpreted_node.package_name, num_buckets); + let reinterpreted_node = PackageTableNode::from_bytes(&node.as_bytes()).unwrap(); assert_eq!(node, &reinterpreted_node); }