From b4af3bc956aeada18783b1c8b258cf2c8a0913d3 Mon Sep 17 00:00:00 2001 From: "Brian L. Troutwine" Date: Tue, 29 Oct 2024 12:30:19 -0700 Subject: [PATCH] Avoid some allocations, hashes that aren't needed This commit removes allocations in the FUSE component that don't need to exist and adjust the interior hashmap to use FxHashMap, avoiding a secure hash in favor of a fast one. Also, directories now keep their children in a BTreeSet so that readdir is always in the same order when called. Signed-off-by: Brian L. Troutwine --- examples/lading-logrotatefs.yaml | 2 +- lading/src/generator/file_gen/logrotate_fs.rs | 72 ++++++++++--------- .../generator/file_gen/logrotate_fs/model.rs | 48 ++++++++----- 3 files changed, 68 insertions(+), 54 deletions(-) diff --git a/examples/lading-logrotatefs.yaml b/examples/lading-logrotatefs.yaml index 9a72144d6..c673e0882 100644 --- a/examples/lading-logrotatefs.yaml +++ b/examples/lading-logrotatefs.yaml @@ -8,7 +8,7 @@ generator: total_rotations: 4 max_depth: 0 variant: "ascii" - bytes_per_second: 10MB + bytes_per_second: 1.3MB maximum_prebuild_cache_size_bytes: 1GB mount_point: /tmp/logrotate diff --git a/lading/src/generator/file_gen/logrotate_fs.rs b/lading/src/generator/file_gen/logrotate_fs.rs index 75fb5d203..6ee710442 100644 --- a/lading/src/generator/file_gen/logrotate_fs.rs +++ b/lading/src/generator/file_gen/logrotate_fs.rs @@ -342,52 +342,56 @@ impl Filesystem for LogrotateFS { state.advance_time(tick); let root_inode = state.root_inode(); + let mut entry_offset = 0; - // TODO building up a vec of entries here to handle offset really does - // suggest that the model needs to be exposed in such a way that this - // needn't be done. - // - // Ah, actually, the right buffer to push into is reply.add. There's no - // need for `entries` at all. - let mut entries = Vec::new(); + // Entry 0: "." + if entry_offset >= offset + && reply.add(ino, entry_offset + 1, fuser::FileType::Directory, ".") + { + reply.ok(); + return; + } + entry_offset += 1; - // '.' and '..' - entries.push((ino, fuser::FileType::Directory, ".".to_string())); + // Entry 1: ".." when applicable if ino != root_inode as u64 { - let parent_ino = state - .get_parent_inode(ino as usize) - .expect("inode must have parent"); - entries.push(( - parent_ino as u64, - fuser::FileType::Directory, - "..".to_string(), - )); + if entry_offset >= offset { + let parent_ino = state + .get_parent_inode(ino as usize) + .expect("inode must have parent"); + if reply.add( + parent_ino as u64, + entry_offset + 1, + fuser::FileType::Directory, + "..", + ) { + reply.ok(); + return; + } + } + entry_offset += 1; } - // remaining children + // Child entries, returned in inode order by `State::readdir` if let Some(child_inodes) = state.readdir(ino as usize) { - for child_ino in child_inodes { - let file_type = state - .get_file_type(*child_ino) - .expect("inode must have file type"); - let child_name = state.get_name(*child_ino).expect("inode must have a name"); - entries.push((*child_ino as u64, file_type, child_name.to_string())); + for &child_ino in child_inodes { + if entry_offset >= offset { + let file_type = state + .get_file_type(child_ino) + .expect("inode must have file type"); + let child_name = state.get_name(child_ino).expect("inode must have a name"); + if reply.add(child_ino as u64, entry_offset + 1, file_type, child_name) { + reply.ok(); + return; + } + } + entry_offset += 1; } } else { reply.error(ENOENT); return; } - let mut idx = offset as usize; - while idx < entries.len() { - let (inode, file_type, name) = &entries[idx]; - let next_offset = (idx + 1) as i64; - if reply.add(*inode, next_offset, *file_type, name) { - // Buffer is full, exit the loop - break; - } - idx += 1; - } reply.ok(); } diff --git a/lading/src/generator/file_gen/logrotate_fs/model.rs b/lading/src/generator/file_gen/logrotate_fs/model.rs index 040c4dc2b..a98dfe8d6 100644 --- a/lading/src/generator/file_gen/logrotate_fs/model.rs +++ b/lading/src/generator/file_gen/logrotate_fs/model.rs @@ -1,11 +1,11 @@ //! Model the internal logic of a logrotate filesystem. -use std::collections::{HashMap, HashSet}; - use bytes::Bytes; use lading_payload::block; use metrics::counter; use rand::Rng; +use rustc_hash::FxHashMap; +use std::collections::BTreeSet; /// Time representation of the model pub(crate) type Tick = u64; @@ -249,12 +249,21 @@ impl File { /// Model representation of a `Directory`. Contains children are `Directory` /// instances or `File` instances. Root directory will not have a `parent`. -#[derive(Debug)] +#[derive(Debug, Default)] pub(crate) struct Directory { - children: HashSet, + children: BTreeSet, parent: Option, } +impl Directory { + fn new(parent: Option) -> Self { + Self { + children: BTreeSet::default(), + parent, + } + } +} + /// A filesystem object, either a `File` or a `Directory`. #[derive(Debug)] pub(crate) enum Node { @@ -278,7 +287,7 @@ pub(crate) enum Node { /// filesystem. It does not contain any bytes, the caller must maintain this /// themselves. pub(crate) struct State { - nodes: HashMap, + nodes: FxHashMap, root_inode: Inode, now: Tick, block_cache: block::Cache, @@ -288,6 +297,7 @@ pub(crate) struct State { group_names: Vec>, next_inode: Inode, next_file_handle: u64, + inode_scratch: Vec, } impl std::fmt::Debug for State { @@ -297,6 +307,7 @@ impl std::fmt::Debug for State { .field("root_inode", &self.root_inode) .field("now", &self.now) // intentionally leaving out block_cache + // intentionally leaving out inode_scratch .field("max_rotations", &self.max_rotations) .field("max_bytes_per_file", &self.max_bytes_per_file) .field("group_names", &self.group_names) @@ -349,12 +360,9 @@ impl State { R: Rng, { let root_inode: Inode = 1; // `/` - let mut nodes = HashMap::new(); + let mut nodes = FxHashMap::default(); - let root_dir = Directory { - children: HashSet::new(), - parent: None, - }; + let root_dir = Directory::default(); nodes.insert( root_inode, Node::Directory { @@ -373,6 +381,7 @@ impl State { group_names: Vec::new(), next_inode: 2, next_file_handle: 0, + inode_scratch: Vec::with_capacity(concurrent_logs as usize), }; if concurrent_logs == 0 { @@ -435,10 +444,7 @@ impl State { let new_inode = state.next_inode; state.next_inode += 1; - let new_dir = Directory { - children: HashSet::new(), - parent: Some(current_inode), - }; + let new_dir = Directory::new(Some(current_inode)); state.nodes.insert( new_inode, Node::Directory { @@ -536,8 +542,11 @@ impl State { fn advance_time_inner(&mut self, now: Tick) { assert!(now >= self.now); - let mut inodes: Vec = self.nodes.keys().copied().collect(); - for inode in inodes.drain(..) { + for inode in self.nodes.keys() { + self.inode_scratch.push(*inode); + } + + for inode in self.inode_scratch.drain(..) { let (rotated_inode, parent_inode, bytes_per_tick, group_id, ordinal) = { // If the node pointed to by inode doesn't exist, that's a // catastrophic programming error. We just copied all inode to node @@ -806,14 +815,15 @@ impl State { } } - /// Read inodes from a directory + /// Read inodes from a directory. /// /// Returns None if the inode is a `File`, else returns the hashset of - /// children inodes. + /// children inodes. Guaranteed to be in the same order so long as time does + /// not advance. /// /// Function does not advance time in the model. #[tracing::instrument(skip(self))] - pub(crate) fn readdir(&self, inode: Inode) -> Option<&HashSet> { + pub(crate) fn readdir(&self, inode: Inode) -> Option<&BTreeSet> { if let Some(Node::Directory { dir, .. }) = self.nodes.get(&inode) { Some(&dir.children) } else {