mirror of
https://github.com/storytold/filmcraft.git
synced 2026-10-24 01:58:19 -06:00
fix(cfb): adding a child is O(1), not a scan of every sibling
Writer::add rejected a duplicate name by comparing the new name with every sibling through compare_names, which allocates two Vec<u16> per call, so n children in one storage cost O(n^2) allocating comparisons. An AAF export puts every marker and component in one storage: 20 000 markers took 58 s, 20 000 clips 56 s, 20 000 connected clips 87 s and 100 000 markers never finished (an FCPXML of 1 MB is enough). Doubling the input quadrupled the time; every other exporter stayed linear. Each storage keeps a set of its children's names as sibling order compares them (UTF-16 code units, upper-cased), so the duplicate check is a hash lookup. Same result: case-insensitive, also beyond ASCII, per storage. 20 000 markers now export to AAF in 0.38 s, 100 000 in 2.0 s, 20 000 clips in 0.23 s and 20 000 connected clips in 0.45 s. Tests: adding 40 000 children must take under 3 s (0.02 s now, 21 s before) and the duplicate rules are unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5.5
parent
09aaf9bf15
commit
a711c61840
@@ -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.
|
||||
|
||||
+28
-4
@@ -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<u8>,
|
||||
children: Vec<usize>,
|
||||
/// 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<Vec<u16>>,
|
||||
}
|
||||
|
||||
/// A name as sibling comparison sees it: its UTF-16 code units, upper-cased.
|
||||
fn name_key(name: &str) -> Vec<u16> {
|
||||
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<u8>) -> Result<NodeId> {
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
Reference in New Issue
Block a user