Skip to content

Commit 2fd80ee

Browse files
authored
Avoid some allocations, hashes that aren't needed (#1076)
### What does this PR do? 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.
1 parent d0d870e commit 2fd80ee

File tree

3 files changed

+68
-54
lines changed

3 files changed

+68
-54
lines changed

examples/lading-logrotatefs.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ generator:
88
total_rotations: 4
99
max_depth: 0
1010
variant: "ascii"
11-
bytes_per_second: 10MB
11+
bytes_per_second: 1.3MB
1212
maximum_prebuild_cache_size_bytes: 1GB
1313
mount_point: /tmp/logrotate
1414

lading/src/generator/file_gen/logrotate_fs.rs

Lines changed: 38 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -342,52 +342,56 @@ impl Filesystem for LogrotateFS {
342342
state.advance_time(tick);
343343

344344
let root_inode = state.root_inode();
345+
let mut entry_offset = 0;
345346

346-
// TODO building up a vec of entries here to handle offset really does
347-
// suggest that the model needs to be exposed in such a way that this
348-
// needn't be done.
349-
//
350-
// Ah, actually, the right buffer to push into is reply.add. There's no
351-
// need for `entries` at all.
352-
let mut entries = Vec::new();
347+
// Entry 0: "."
348+
if entry_offset >= offset
349+
&& reply.add(ino, entry_offset + 1, fuser::FileType::Directory, ".")
350+
{
351+
reply.ok();
352+
return;
353+
}
354+
entry_offset += 1;
353355

354-
// '.' and '..'
355-
entries.push((ino, fuser::FileType::Directory, ".".to_string()));
356+
// Entry 1: ".." when applicable
356357
if ino != root_inode as u64 {
357-
let parent_ino = state
358-
.get_parent_inode(ino as usize)
359-
.expect("inode must have parent");
360-
entries.push((
361-
parent_ino as u64,
362-
fuser::FileType::Directory,
363-
"..".to_string(),
364-
));
358+
if entry_offset >= offset {
359+
let parent_ino = state
360+
.get_parent_inode(ino as usize)
361+
.expect("inode must have parent");
362+
if reply.add(
363+
parent_ino as u64,
364+
entry_offset + 1,
365+
fuser::FileType::Directory,
366+
"..",
367+
) {
368+
reply.ok();
369+
return;
370+
}
371+
}
372+
entry_offset += 1;
365373
}
366374

367-
// remaining children
375+
// Child entries, returned in inode order by `State::readdir`
368376
if let Some(child_inodes) = state.readdir(ino as usize) {
369-
for child_ino in child_inodes {
370-
let file_type = state
371-
.get_file_type(*child_ino)
372-
.expect("inode must have file type");
373-
let child_name = state.get_name(*child_ino).expect("inode must have a name");
374-
entries.push((*child_ino as u64, file_type, child_name.to_string()));
377+
for &child_ino in child_inodes {
378+
if entry_offset >= offset {
379+
let file_type = state
380+
.get_file_type(child_ino)
381+
.expect("inode must have file type");
382+
let child_name = state.get_name(child_ino).expect("inode must have a name");
383+
if reply.add(child_ino as u64, entry_offset + 1, file_type, child_name) {
384+
reply.ok();
385+
return;
386+
}
387+
}
388+
entry_offset += 1;
375389
}
376390
} else {
377391
reply.error(ENOENT);
378392
return;
379393
}
380394

381-
let mut idx = offset as usize;
382-
while idx < entries.len() {
383-
let (inode, file_type, name) = &entries[idx];
384-
let next_offset = (idx + 1) as i64;
385-
if reply.add(*inode, next_offset, *file_type, name) {
386-
// Buffer is full, exit the loop
387-
break;
388-
}
389-
idx += 1;
390-
}
391395
reply.ok();
392396
}
393397

lading/src/generator/file_gen/logrotate_fs/model.rs

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
//! Model the internal logic of a logrotate filesystem.
22
3-
use std::collections::{HashMap, HashSet};
4-
53
use bytes::Bytes;
64
use lading_payload::block;
75
use metrics::counter;
86
use rand::Rng;
7+
use rustc_hash::FxHashMap;
8+
use std::collections::BTreeSet;
99

1010
/// Time representation of the model
1111
pub(crate) type Tick = u64;
@@ -249,12 +249,21 @@ impl File {
249249

250250
/// Model representation of a `Directory`. Contains children are `Directory`
251251
/// instances or `File` instances. Root directory will not have a `parent`.
252-
#[derive(Debug)]
252+
#[derive(Debug, Default)]
253253
pub(crate) struct Directory {
254-
children: HashSet<Inode>,
254+
children: BTreeSet<Inode>,
255255
parent: Option<Inode>,
256256
}
257257

258+
impl Directory {
259+
fn new(parent: Option<Inode>) -> Self {
260+
Self {
261+
children: BTreeSet::default(),
262+
parent,
263+
}
264+
}
265+
}
266+
258267
/// A filesystem object, either a `File` or a `Directory`.
259268
#[derive(Debug)]
260269
pub(crate) enum Node {
@@ -278,7 +287,7 @@ pub(crate) enum Node {
278287
/// filesystem. It does not contain any bytes, the caller must maintain this
279288
/// themselves.
280289
pub(crate) struct State {
281-
nodes: HashMap<Inode, Node>,
290+
nodes: FxHashMap<Inode, Node>,
282291
root_inode: Inode,
283292
now: Tick,
284293
block_cache: block::Cache,
@@ -288,6 +297,7 @@ pub(crate) struct State {
288297
group_names: Vec<Vec<String>>,
289298
next_inode: Inode,
290299
next_file_handle: u64,
300+
inode_scratch: Vec<Inode>,
291301
}
292302

293303
impl std::fmt::Debug for State {
@@ -297,6 +307,7 @@ impl std::fmt::Debug for State {
297307
.field("root_inode", &self.root_inode)
298308
.field("now", &self.now)
299309
// intentionally leaving out block_cache
310+
// intentionally leaving out inode_scratch
300311
.field("max_rotations", &self.max_rotations)
301312
.field("max_bytes_per_file", &self.max_bytes_per_file)
302313
.field("group_names", &self.group_names)
@@ -349,12 +360,9 @@ impl State {
349360
R: Rng,
350361
{
351362
let root_inode: Inode = 1; // `/`
352-
let mut nodes = HashMap::new();
363+
let mut nodes = FxHashMap::default();
353364

354-
let root_dir = Directory {
355-
children: HashSet::new(),
356-
parent: None,
357-
};
365+
let root_dir = Directory::default();
358366
nodes.insert(
359367
root_inode,
360368
Node::Directory {
@@ -373,6 +381,7 @@ impl State {
373381
group_names: Vec::new(),
374382
next_inode: 2,
375383
next_file_handle: 0,
384+
inode_scratch: Vec::with_capacity(concurrent_logs as usize),
376385
};
377386

378387
if concurrent_logs == 0 {
@@ -435,10 +444,7 @@ impl State {
435444
let new_inode = state.next_inode;
436445
state.next_inode += 1;
437446

438-
let new_dir = Directory {
439-
children: HashSet::new(),
440-
parent: Some(current_inode),
441-
};
447+
let new_dir = Directory::new(Some(current_inode));
442448
state.nodes.insert(
443449
new_inode,
444450
Node::Directory {
@@ -536,8 +542,11 @@ impl State {
536542
fn advance_time_inner(&mut self, now: Tick) {
537543
assert!(now >= self.now);
538544

539-
let mut inodes: Vec<Inode> = self.nodes.keys().copied().collect();
540-
for inode in inodes.drain(..) {
545+
for inode in self.nodes.keys() {
546+
self.inode_scratch.push(*inode);
547+
}
548+
549+
for inode in self.inode_scratch.drain(..) {
541550
let (rotated_inode, parent_inode, bytes_per_tick, group_id, ordinal) = {
542551
// If the node pointed to by inode doesn't exist, that's a
543552
// catastrophic programming error. We just copied all inode to node
@@ -806,14 +815,15 @@ impl State {
806815
}
807816
}
808817

809-
/// Read inodes from a directory
818+
/// Read inodes from a directory.
810819
///
811820
/// Returns None if the inode is a `File`, else returns the hashset of
812-
/// children inodes.
821+
/// children inodes. Guaranteed to be in the same order so long as time does
822+
/// not advance.
813823
///
814824
/// Function does not advance time in the model.
815825
#[tracing::instrument(skip(self))]
816-
pub(crate) fn readdir(&self, inode: Inode) -> Option<&HashSet<Inode>> {
826+
pub(crate) fn readdir(&self, inode: Inode) -> Option<&BTreeSet<Inode>> {
817827
if let Some(Node::Directory { dir, .. }) = self.nodes.get(&inode) {
818828
Some(&dir.children)
819829
} else {

0 commit comments

Comments
 (0)