diff --git a/crates/cfb/README.md b/crates/cfb/README.md index b0f0d8741..6b234b3ed 100644 --- a/crates/cfb/README.md +++ b/crates/cfb/README.md @@ -50,3 +50,6 @@ chain is contiguous. Sibling trees are balanced binary search trees in [MS-CFB] bytes) and multi-sector streams and up to 40 siblings; red-black invariants of every sibling tree; a 7.5 MB stream that needs DIFAT sectors; Unicode and case-insensitive names; truncation at every 29 bytes and 1500 random mutations never panic and only return complete streams. +The writer rejects a duplicate sibling name with an O(1) hash lookup on the upper-cased name (a scan over the +siblings made n children cost O(n^2): 20 000 AAF markers took a minute); `adding_many_children_is_not_quadratic` +and `duplicate_names_are_still_rejected` (`tests/writer_scale.rs`) cover it. diff --git a/crates/cfb/src/write.rs b/crates/cfb/src/write.rs index 923e1aa38..8a4ad4fbe 100644 --- a/crates/cfb/src/write.rs +++ b/crates/cfb/src/write.rs @@ -5,6 +5,8 @@ //! when the FAT needs more than the 109 header slots. Every chain is contiguous. Sibling trees are //! balanced binary search trees coloured as valid red-black trees (§2.6.4). +use std::collections::HashSet; + use crate::{DIFSECT, ENDOFCHAIN, FATSECT, FREESECT, MINI_SECTOR_SIZE, MINI_STREAM_CUTOFF, NOSTREAM, Result, SIGNATURE, compare_names, validate_name}; /// File format version. @@ -44,6 +46,15 @@ struct Node { clsid: [u8; 16], data: Vec, children: Vec, + /// The children's names, upper-cased as sibling order compares them: an O(1) duplicate check. + /// (Scanning every sibling with `compare_names`, which allocates, made adding n children O(n^2): + /// 20 000 AAF markers took a minute.) + child_keys: HashSet>, +} + +/// A name as sibling comparison sees it: its UTF-16 code units, upper-cased. +fn name_key(name: &str) -> Vec { + name.encode_utf16().map(crate::upper).collect() } /// Builds a compound file in memory. @@ -64,7 +75,17 @@ impl Writer { pub const ROOT: NodeId = NodeId(0); pub fn new(version: Version) -> Self { - Writer { version, nodes: vec![Node { name: "Root Entry".into(), kind: Kind::Root, clsid: [0; 16], data: Vec::new(), children: Vec::new() }] } + Writer { + version, + nodes: vec![Node { + name: "Root Entry".into(), + kind: Kind::Root, + clsid: [0; 16], + data: Vec::new(), + children: Vec::new(), + child_keys: HashSet::new(), + }], + } } fn add(&mut self, parent: NodeId, name: &str, kind: Kind, clsid: [u8; 16], data: Vec) -> Result { @@ -73,12 +94,15 @@ impl Writer { if p.kind == Kind::Stream { return Err(crate::Error::Invalid(format!("{:?} is a stream, not a storage", p.name))); } - if p.children.iter().any(|&c| compare_names(&self.nodes[c].name, name).is_eq()) { + let key = name_key(name); + if p.child_keys.contains(&key) { return Err(crate::Error::BadName(format!("duplicate name {name}"))); } let id = self.nodes.len(); - self.nodes.push(Node { name: name.to_string(), kind, clsid, data, children: Vec::new() }); - self.nodes[parent.0].children.push(id); + self.nodes.push(Node { name: name.to_string(), kind, clsid, data, children: Vec::new(), child_keys: HashSet::new() }); + let p = &mut self.nodes[parent.0]; + p.children.push(id); + p.child_keys.insert(key); Ok(NodeId(id)) } diff --git a/crates/cfb/tests/writer_scale.rs b/crates/cfb/tests/writer_scale.rs new file mode 100644 index 000000000..47c40f9a2 --- /dev/null +++ b/crates/cfb/tests/writer_scale.rs @@ -0,0 +1,33 @@ +//! The writer scales: adding many children to one storage, and the duplicate-name rules that check keeps. + +use filmcraft_cfb::{Version, Writer}; + +/// Adding a child compared its name with every sibling (allocating twice per comparison), so n children +/// cost O(n^2): 20 000 AAF markers in one storage took about a minute. The duplicate check is a hash +/// lookup now. 40 000 plain names take milliseconds now and about 18 s before (a minute for real marker +/// objects), so the limit separates them by orders of magnitude on any machine. +#[test] +fn adding_many_children_is_not_quadratic() { + let mut w = Writer::new(Version::V3); + let start = std::time::Instant::now(); + for i in 0..40_000 { + w.stream(Writer::ROOT, &format!("{i}"), vec![1]).expect("unique name"); + } + assert!(start.elapsed() < std::time::Duration::from_secs(3), "{:?}", start.elapsed()); +} + +/// The faster duplicate check still means what sibling comparison means: case-insensitive, also beyond ASCII, +/// per storage (the same name in another storage is fine), and a name that differs only in length is distinct. +#[test] +fn duplicate_names_are_still_rejected() { + let mut w = Writer::new(Version::V3); + let a = w.storage(Writer::ROOT, "Alpha", [0; 16]).unwrap(); + assert!(w.stream(Writer::ROOT, "ALPHA", vec![]).is_err(), "case-insensitive"); + assert!(w.stream(Writer::ROOT, "alpha", vec![]).is_err()); + assert!(w.stream(Writer::ROOT, "Alph", vec![]).is_ok()); + assert!(w.stream(Writer::ROOT, "Alpha2", vec![]).is_ok()); + assert!(w.stream(a, "Alpha", vec![]).is_ok(), "same name in another storage"); + assert!(w.stream(a, "Alpha", vec![]).is_err()); + w.stream(Writer::ROOT, "Ünï", vec![]).unwrap(); + assert!(w.stream(Writer::ROOT, "ÜNÏ", vec![]).is_err(), "non-ASCII case"); +}