Merge "aconfig: remove bucket_index from PackageTableNode/FlagTableNode struct" into main am: 4767d785c1

Original change: https://android-review.googlesource.com/c/platform/build/+/2932354

Change-Id: I59df9c45001095530555a1fa8e39fea45189c6bb
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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<Vec<FlagTableNode>> {
-    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::<Result<Vec<_>>>()
+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<Vec<Self>> {
+        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::<Result<Vec<_>>>()
+    }
 }
 
 pub fn create_flag_table(container: &str, packages: &[FlagPackage]) -> Result<FlagTable> {
@@ -73,44 +91,48 @@ pub fn create_flag_table(container: &str, packages: &[FlagPackage]) -> Result<Fl
     let num_flags = packages.iter().map(|pkg| pkg.boolean_flags.len() as u32).sum();
     let num_buckets = get_table_size(num_flags)?;
 
-    let mut table = FlagTable {
-        header: new_header(container, num_flags),
-        buckets: vec![None; num_buckets as usize],
-        nodes: packages
-            .iter()
-            .map(|pkg| create_nodes(pkg, num_buckets))
-            .collect::<Result<Vec<_>>>()?
-            .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::<Result<Vec<_>>>()?
+        .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::<usize>() 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::<usize>() 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<u32>,
-        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<FlagTableNode> = &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));
     }
 }

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::<usize>() 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::<usize>() 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);
     }

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<u32>,
-    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::<Result<Vec<_>>>()?;
 
         let table = Self { header, buckets, nodes };
         Ok(table)
@@ -203,7 +199,6 @@ mod tests {
             flag_type: u16,
             flag_id: u16,
             next_offset: Option<u32>,
-            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<FlagTableNode> = &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);
         }
 

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<u32>,
-    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::<Result<Vec<_>>>()?;
 
         let table = Self { header, buckets, nodes };
         Ok(table)
@@ -166,7 +170,7 @@ pub fn find_package_offset(buf: &[u8], package: &str) -> Result<Option<PackageOf
     }
 
     let num_buckets = (interpreted_header.node_offset - interpreted_header.bucket_offset) / 4;
-    let bucket_index = get_bucket_index(&package, num_buckets);
+    let bucket_index = PackageTableNode::find_bucket_index(&package, num_buckets);
 
     let mut pos = (interpreted_header.bucket_offset + 4 * bucket_index) as usize;
     let mut package_node_offset = read_u32_from_bytes(buf, &mut pos)? as usize;
@@ -210,21 +214,18 @@ mod tests {
             package_id: 1,
             boolean_offset: 3,
             next_offset: None,
-            bucket_index: 0,
         };
         let second_node = PackageTableNode {
             package_name: String::from("com.android.aconfig.storage.test_1"),
             package_id: 0,
             boolean_offset: 0,
             next_offset: Some(158),
-            bucket_index: 3,
         };
         let third_node = PackageTableNode {
             package_name: String::from("com.android.aconfig.storage.test_4"),
             package_id: 2,
             boolean_offset: 6,
             next_offset: None,
-            bucket_index: 3,
         };
         let nodes = vec![first_node, second_node, third_node];
         Ok(PackageTable { header, buckets, nodes })
@@ -241,11 +242,8 @@ mod tests {
         assert_eq!(header, &reinterpreted_header.unwrap());
 
         let nodes: &Vec<PackageTableNode> = &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);
         }