From 315e83b88c8e06f308fbbd1826ddc3c864e4ba18 Mon Sep 17 00:00:00 2001 From: KenM76 Date: Mon, 2 Nov 2026 15:22:50 -0500 Subject: [PATCH] perf(cos): an object that can't be read is searched for once, not on every read Document::try_get kept only objects that loaded. An object whose entry points at something that doesn't parse falls back to a whole-file search (scan_for); that failed too, nothing was kept, and the next read searched the whole file again. A caller that reads such an object repeatedly, such as the window title of a file whose document information dictionary is damaged and that asks for its title to be shown, paid one whole-file search per read, several per frame. The cache now keeps the load result, a failure as well. It is the same map with the same bounds, clears and per-reader privacy, so every place that drops loaded objects (authentication, reconstruction, private scan readers, eviction) drops remembered failures too. Edits are read from the overlay before the cache, as before, so a set or freed number still answers for itself. A failure is kept in nested loads too, as a success already is; only a poisoned lock is never kept. Failures count towards the cache's object caps and hold no bytes of their own. Sharing the map between clones stays sound because entries, data and the stream limit don't change after open. Test: an_unreadable_object_is_searched_for_once counts whole-file searches through a test-only counter (like SECTION_READS): one for eleven reads and a clone's read. With failures not kept it counts 22. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9 --- crates/cos/src/document.rs | 10 +++++---- crates/cos/src/document/objects.rs | 35 +++++++++++++++++------------- crates/cos/src/document/state.rs | 12 +++++++++- crates/cos/src/document/tests.rs | 31 +++++++++++++++++++++++--- 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index 082d1910d1..6e5891d1a4 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -24,10 +24,10 @@ pub(crate) use reader::find; #[cfg(test)] use reader::rfind; pub(crate) use state::ObjectReader; -use state::{ObjStm, Slot}; -pub use state::{ReadState, SourceIdentity}; +use state::{Loaded, ObjStm, Slot}; #[cfg(test)] -use state::{SECTION_READS, objstm_member, owned_bytes}; +use state::{OBJECT_SEARCHES, SECTION_READS, objstm_member, owned_bytes}; +pub use state::{ReadState, SourceIdentity}; use xref::XrefIndex; pub use xref::{Revision, XrefEntry}; @@ -47,7 +47,9 @@ pub struct Document { /// `fill_xref_gaps`. located: Arc>, repair_log: Arc>, - cache: Arc>>>, + /// What loading each file object gave, a failure as well: a damaged object is otherwise + /// searched for across the whole file on every read (`Document::load`). + cache: Arc>>, objstms: Arc>>>, /// What `/JBIG2Globals` streams of the file decode to, by object number: the images of a scan /// share them, and each would otherwise decode them again (see `Document::jbig2_globals`). diff --git a/crates/cos/src/document/objects.rs b/crates/cos/src/document/objects.rs index 2ac18cff03..2ca327de74 100644 --- a/crates/cos/src/document/objects.rs +++ b/crates/cos/src/document/objects.rs @@ -1,5 +1,5 @@ use super::reader::find; -use super::state::{LENGTHS, LoadGuard, ObjStm, ObjectReader, ReadState, Slot, SourceIdentity, objstm_member, owned_bytes}; +use super::state::{LENGTHS, LoadGuard, Loaded, ObjStm, ObjectReader, ReadState, Slot, SourceIdentity, loaded_bytes, objstm_member}; use super::xref::{Revision, XrefEntry}; use super::{ Arc, CosError, Cow, Dict, Document, Lexer, MAX_DECODED, MAX_JBIG2_GLOBALS, ObjRef, Object, Stream, is_whitespace, parse_indirect_shared, @@ -388,20 +388,23 @@ impl Document { Some(Slot::Freed(_)) => return Ok(Arc::new(Object::Null)), None => {} } - if let Some(o) = self.cache.lock().map_err(|_| CosError::Poisoned)?.get(&num) { - return Ok(o.clone()); + if let Some(loaded) = self.cache.lock().map_err(|_| CosError::Poisoned)?.get(&num) { + return loaded.clone(); } // Loading can re-enter (indirect stream `/Length`, object streams); damaged files make // cycles such as `6 0 obj << /Length 6 0 R >>`. Bound the nesting on every path. let _guard = LoadGuard::enter(num).ok_or_else(|| CosError::Syntax { offset: 0, detail: format!("reference cycle while loading object {num}") })?; - let obj = self.load(num, 0)?; - let obj = Arc::new(obj); - // The editing cache is bounded like the scan reader's: re-parsing a file-backed view - // is cheap, and returned objects outlive eviction. - self.cache.lock().map_err(|_| CosError::Poisoned)?.insert(num, obj.clone()); - self.evict_editor_cache_if_over_budget(num, &obj)?; - Ok(obj) + let loaded = self.load(num, 0).map(Arc::new); + // A failure is kept like an object, or every read of a damaged object searches the whole + // file again. A poisoned lock says nothing about the object. + if !matches!(loaded, Err(CosError::Poisoned)) { + // The editing cache is bounded like the scan reader's: re-parsing a file-backed view + // is cheap, and returned objects outlive eviction. + self.cache.lock().map_err(|_| CosError::Poisoned)?.insert(num, loaded.clone()); + self.evict_editor_cache_if_over_budget(num, &loaded)?; + } + loaded } /// Bounds for the editing cache: roomier than the scan reader's (128 objects / 8 MB) @@ -430,17 +433,17 @@ impl Document { /// byte budget can double. const EDITOR_LARGE_OBJECT: usize = 256 * 1024; - fn evict_editor_cache_if_over_budget(&self, fresh_num: u32, fresh: &Object) -> Result<(), CosError> { + fn evict_editor_cache_if_over_budget(&self, fresh_num: u32, fresh: &Loaded) -> Result<(), CosError> { // Fast path: the full fold is O(cache) per insert, so skip it for a small newcomer // below the count cap. `len` is copied out first; nothing here holds the lock. let len = self.cache.lock().map_err(|_| CosError::Poisoned)?.len(); - if len <= Self::EDITOR_CACHE_OBJECTS && owned_bytes(fresh, &self.data, 0) <= Self::EDITOR_LARGE_OBJECT { + if len <= Self::EDITOR_CACHE_OBJECTS && loaded_bytes(fresh, &self.data) <= Self::EDITOR_LARGE_OBJECT { return Ok(()); } // Walk a snapshot off-lock; re-lock only to evict. - let live: Vec<(u32, Arc)> = self.cache.lock().map_err(|_| CosError::Poisoned)?.iter().map(|(n, o)| (*n, o.clone())).collect(); + let live: Vec<(u32, Loaded)> = self.cache.lock().map_err(|_| CosError::Poisoned)?.iter().map(|(n, o)| (*n, o.clone())).collect(); let data = &self.data; - let owned: usize = live.iter().fold(0usize, |n, (_, o)| n.saturating_add(owned_bytes(o, data, 0))); + let owned: usize = live.iter().fold(0usize, |n, (_, o)| n.saturating_add(loaded_bytes(o, data))); if live.len() <= Self::EDITOR_CACHE_OBJECTS && owned <= Self::EDITOR_CACHE_BYTES { return Ok(()); } @@ -450,7 +453,7 @@ impl Document { let mut cache = self.cache.lock().map_err(|_| CosError::Poisoned)?; if cache.len() <= Self::EDITOR_CACHE_OBJECTS { let data = &self.data; - let owned: usize = cache.values().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, data, 0))); + let owned: usize = cache.values().fold(0usize, |n, o| n.saturating_add(loaded_bytes(o, data))); if owned <= Self::EDITOR_CACHE_BYTES { return Ok(()); } @@ -602,6 +605,8 @@ impl Document { /// Last-resort lookup: scan the file for `num G obj`. fn scan_for(&self, num: u32) -> Option<(ObjRef, Object)> { + #[cfg(test)] + super::OBJECT_SEARCHES.with(|c| c.set(c.get() + 1)); let needle = format!("{num} "); let data = &self.data; let mut found = None; diff --git a/crates/cos/src/document/state.rs b/crates/cos/src/document/state.rs index b4323f982e..121a59c2e7 100644 --- a/crates/cos/src/document/state.rs +++ b/crates/cos/src/document/state.rs @@ -30,6 +30,14 @@ impl LoadGuard { } } +/// What loading a file object gave, kept in the document's cache either way. +pub(super) type Loaded = Result, CosError>; + +/// The bytes a load result holds of its own ([`owned_bytes`]); a failure holds none. +pub(super) fn loaded_bytes(loaded: &Loaded, data: &Arc>) -> usize { + loaded.as_ref().map_or(0, |o| owned_bytes(o, data, 0)) +} + impl Drop for LoadGuard { fn drop(&mut self) { let outermost = LOADING.with(|l| { @@ -165,7 +173,7 @@ impl ObjectReader { // have cached large objects. if let Ok(mut cache) = self.document.cache.lock() { let data = &self.document.data; - if cache.len() >= Self::OBJECTS || cache.values().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, data, 0))) > Self::STREAM_BYTES { + if cache.len() >= Self::OBJECTS || cache.values().fold(0usize, |n, o| n.saturating_add(loaded_bytes(o, data))) > Self::STREAM_BYTES { cache.clear(); } } @@ -177,6 +185,8 @@ impl ObjectReader { thread_local! { /// Cross-reference sections read on this thread. pub(super) static SECTION_READS: std::cell::Cell = const { std::cell::Cell::new(0) }; + /// Whole-file searches for an object (`Document::scan_for`) on this thread. + pub(super) static OBJECT_SEARCHES: std::cell::Cell = const { std::cell::Cell::new(0) }; } /// Nesting [`owned_bytes`] walks: past the parser's own nesting limit, so every parsed object diff --git a/crates/cos/src/document/tests.rs b/crates/cos/src/document/tests.rs index cca4b87326..3cebe0136d 100644 --- a/crates/cos/src/document/tests.rs +++ b/crates/cos/src/document/tests.rs @@ -588,6 +588,31 @@ fn an_entry_that_misses_its_object_is_found_by_searching_but_is_not_exact() { assert!(doc.xref_entry_is_exact(1) && doc.xref_entry_is_exact(2)); } +/// An object that can't be read at its entry or anywhere else is searched for in the whole file +/// once: every later read, in a clone too, gives the same error without searching again. An +/// edit of that number still answers for it. +#[test] +fn an_unreadable_object_is_searched_for_once() { + let bodies = ["<< /Type /Catalog /Pages 2 0 R >>", "<< /Type /Pages /Kids [] /Count 0 >>", "<< /Odd Key(x) >>"]; + let doc = Document::open(Arc::new(build(&bodies, "/Root 1 0 R /Info 3 0 R"))).unwrap(); + // Counted wherever the whole-file search is done (`scan_for`). + OBJECT_SEARCHES.with(|c| c.set(0)); + let first = doc.try_get(3); + assert!(first.is_err(), "{first:?}"); + for _ in 0..10 { + assert_eq!(doc.try_get(3), first); + assert_eq!(*doc.get(ObjRef::new(3, 0)), Object::Null); + } + let mut edited = doc.clone(); + assert_eq!(edited.try_get(3), first); + assert_eq!(OBJECT_SEARCHES.with(|c| c.get()), 1, "searches for an object that isn't there"); + edited.set(ObjRef::new(3, 0), Object::Int(7)); + assert_eq!(edited.try_get(3).as_deref(), Ok(&Object::Int(7))); + assert_eq!(doc.try_get(3), first, "the edit is the clone's alone"); + edited.free(ObjRef::new(3, 0)); + assert_eq!(edited.try_get(3).as_deref(), Ok(&Object::Null)); +} + /// A classic table with `/Size` rows and object 0's free entry first, its subsection numbered /// `1 n` instead of `0 n` (#2676): every row is one number off. fn misnumbered_table(start: u32) -> Vec { @@ -1109,7 +1134,7 @@ fn temporary_reader_keeps_file_views_and_evicts_data_of_its_own() { // Data of its own (decrypted streams, strings) counts: past the budget the cache is // cleared after the next read, including a failed one. let owned = Object::Stream(crate::Stream::from_raw(Dict::new(), vec![b'y'; ObjectReader::STREAM_BYTES + 1])); - reader.document.cache.lock().unwrap().insert(3, Arc::new(owned)); + reader.document.cache.lock().unwrap().insert(3, Ok(Arc::new(owned))); assert_eq!(*reader.get(ObjRef::new(99, 0)), Object::Null); assert!(reader.document.cache.lock().unwrap().is_empty(), "owned data over budget is evicted"); } @@ -1198,8 +1223,8 @@ fn editor_cache_counts_deeply_nested_strings() { let Object::Ref(reference) = reference else { panic!("indirect object") }; drop(doc.get(*reference)); let cache = doc.cache.lock().unwrap(); - let owned = cache.values().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, &doc.data, 0))); - let strings = cache.values().filter(|o| o.as_array().is_some()).count(); + let owned = cache.values().flatten().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, &doc.data, 0))); + let strings = cache.values().flatten().filter(|o| o.as_array().is_some()).count(); assert!(strings << 20 <= Document::EDITOR_CACHE_BYTES + (1 << 20), "{strings} MiB strings cached, {owned} bytes counted"); } }