From 9644aeb9e2f9719949fb757f07c9b062c3b91c05 Mon Sep 17 00:00:00 2001 From: Chiranjeevi U <244287281+chiranjeevi-max@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:16:19 +0530 Subject: [PATCH] Cap unbounded editing and text caches; index form fields once (#1115) * Cap editing and text-layer caches that grew without bound The cos editing cache kept every touched object until save, so editing a large document pinned its whole object graph. Bound it like the scan reader: 1024 objects / 32 MB of owned bytes, 256 object streams / 32 MB, evicting to the newcomer. Returned objects outlive eviction, and clones share the caches, so this only costs re-parsing. The viewer text cache had the same shape: every visited page's text stayed for the session. Retain visible, selected and matched pages plus a 24-page prefetch window each frame; evicted pages re-extract on revisit. Skipped while a search is active, where demand still wants every page. Failures are never evicted: they cost nothing to remember. cli info on a 1024-page/147 KB file (debug build, same host/command): before 66.7 MB RSS, after 54.0 MB RSS (3 runs, tight). Tests: editor count bound, byte bound, snapshot-clone reads (cos); evict-far/keep-visible+matched+selection, search-active keep-all (canvas). All mutation-checked: each bound goes red when disabled. Signed-off-by: Chiranjeevi U <244287281+chiranjeevi-max@users.noreply.github.com> * Index form fields once per refresh instead of once per field The form-scope refresh ran a linear find over all fields for every field, O(F^2) on each keystroke; forms reach 10k fields. Build one name index up front, O(F). entry().or_insert() keeps the old first-match semantics for repeated field names. New test with two same-named widgets pins first-wins and goes red under insert. 141 engine tests green, clippy warnings and perf lints clean. Signed-off-by: Chiranjeevi U <244287281+chiranjeevi-max@users.noreply.github.com> --------- Signed-off-by: Chiranjeevi U <244287281+chiranjeevi-max@users.noreply.github.com> --- crates/cos/src/document.rs | 144 +++++++++++++++++++++++++++++++++++ crates/engine/src/lib.rs | 9 ++- crates/engine/src/tests.rs | 31 ++++++++ crates/ui-egui/src/canvas.rs | 86 +++++++++++++++++++++ 4 files changed, 269 insertions(+), 1 deletion(-) diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index e2c2b219..afbbbbd8 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -145,6 +145,8 @@ impl ObjectReader { pub(crate) fn try_get(&mut self, reference: ObjRef) -> Result, CosError> { let object = self.document.try_get(reference.num); + // A missed eviction here only costs a re-parse, so never fail the read on a poisoned + // lock. The editing-cache evictors propagate instead; they guard user edits. if let Ok(mut streams) = self.document.objstms.lock() { // Only insertion changes this private cache between reads. Most neighboring // objects reuse a decoded stream: do not sum the whole store for every object. @@ -607,10 +609,81 @@ impl Document { 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) } + /// Bounds for the editing cache: roomier than the scan reader's (128 objects / 8 MB) + /// so interactive editing does not thrash, but still capped so touching a large + /// document cannot pin its whole object graph. Only owned (decrypted/copied) bytes + /// count; file-backed views re-parse for free. + /// + /// Worst case per document: input bytes + up to `EDITOR_CACHE_OBJECTS` × + /// `EDITOR_LARGE_OBJECT` (1024 × 256 KB = 256 MB) of small objects between full + /// accounting passes + `EDITOR_OBJSTM_BYTES` (32 MB) + unsaved `overlay` edits. + /// The count cap always holds; byte accounting between passes is approximate. + /// Clones share these caches, so one clone's eviction drops another's hot set; + /// that only costs re-parsing, never correctness. Eviction keeps just the newcomer: + /// file-backed views re-parse cheaply and editor working sets stay far below 1024. + const EDITOR_CACHE_OBJECTS: usize = 1024; + const EDITOR_CACHE_BYTES: usize = 32 * 1024 * 1024; + /// Object streams decode to megabytes each, so fewer are kept than plain objects. + const EDITOR_OBJSTM_OBJECTS: usize = 256; + const EDITOR_OBJSTM_BYTES: usize = 32 * 1024 * 1024; + /// Newcomers above this force a full accounting pass even below the count cap: + /// `EDITOR_CACHE_BYTES / 128`, so 128 large newcomers always trigger a pass before the + /// 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> { + // 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 { + 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 data = &self.data; + let owned: usize = live.iter().fold(0usize, |n, (_, o)| n.saturating_add(owned_bytes(o, data, 0))); + if live.len() <= Self::EDITOR_CACHE_OBJECTS && owned <= Self::EDITOR_CACHE_BYTES { + return Ok(()); + } + // Re-check under the final lock: another thread may have inserted or evicted since + // the snapshot. Keep its newcomers, and skip the eviction if back within budget. + let snapshot: std::collections::HashSet = live.into_iter().map(|(n, _)| n).collect(); + 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))); + if owned <= Self::EDITOR_CACHE_BYTES { + return Ok(()); + } + } + cache.retain(|n, _| *n == fresh_num || !snapshot.contains(n)); + Ok(()) + } + + fn evict_editor_streams_if_over_budget(&self, current: u32) -> Result<(), CosError> { + // The caller inserts `current` first, so retaining it keeps a live entry. + let over = { + let streams = self.objstms.lock().map_err(|_| CosError::Poisoned)?; + let bytes = streams.values().fold(0usize, |bytes, stream| { + bytes + .saturating_add(stream.data.capacity()) + .saturating_add(stream.index.capacity().saturating_mul(std::mem::size_of::<(u32, usize)>())) + }); + bytes > Self::EDITOR_OBJSTM_BYTES || streams.len() > Self::EDITOR_OBJSTM_OBJECTS + }; + if over { + self.objstms.lock().map_err(|_| CosError::Poisoned)?.retain(|n, _| *n == current); + } + Ok(()) + } + /// Visit every current object without filling this document's editing cache. /// /// Full-document searches (for example, standalone timestamps) need to inspect objects @@ -720,7 +793,10 @@ impl Document { index.push((onum.max(0) as u32, first + off.max(0) as usize)); } let stm = Arc::new(ObjStm { index, data: Arc::new(data) }); + // Same bound as the object cache: decoded object streams can be megabytes each. + // Insert first so the retained `num` is actually in the map. self.objstms.lock().map_err(|_| CosError::Poisoned)?.insert(num, stm.clone()); + self.evict_editor_streams_if_over_budget(num)?; Ok(stm) } @@ -1361,6 +1437,74 @@ mod tests { } } + #[test] + fn editor_cache_stays_bounded() { + let mut source = Document::new_empty(); + let objects: Vec<_> = (0..(Document::EDITOR_CACHE_OBJECTS + 64)) + .map(|i| { + let mut value = vec![b'x'; 1024]; + value[..4].copy_from_slice(&(i as u32).to_le_bytes()); + Object::Ref(source.add(Object::String(crate::PdfString::literal(value)))) + }) + .collect(); + source.update_dict(source.root().unwrap(), |d| d.set(b"Extension".to_vec(), Object::Array(objects))).unwrap(); + let bytes = Arc::new(crate::write_full(&source, &crate::SaveOptions::default()).unwrap()); + let doc = Document::open(bytes).unwrap(); + let root = doc.get(doc.root().unwrap()); + let references = root.as_dict().unwrap().get(b"Extension").unwrap().as_array().unwrap().clone(); + let mut held = None; + for (i, reference) in references.iter().enumerate() { + let Object::Ref(reference) = reference else { panic!("indirect object") }; + let object = doc.get(*reference); + assert_eq!(&object.as_string().unwrap().bytes[..4], &(i as u32).to_le_bytes()); + if i == 0 { + held = Some(object); + } + assert!(doc.cache.lock().unwrap().len() <= Document::EDITOR_CACHE_OBJECTS, "editing must not pin the whole object graph"); + // Eviction keeps the newcomer. + assert!(doc.cache.lock().unwrap().contains_key(&reference.num), "eviction must keep the newcomer"); + } + // Returned objects survive eviction, through both handles of the shared cache. + assert_eq!(&held.unwrap().as_string().unwrap().bytes[..4], &0u32.to_le_bytes()); + let snapshot = doc.clone(); + let first = references[0].as_ref().unwrap(); + assert_eq!(&snapshot.get(first).as_string().unwrap().bytes[..4], &0u32.to_le_bytes()); + } + + #[test] + fn editor_cache_byte_bound_fires() { + // 40 × 1 MB strings: far below the count cap, above the 32 MB byte budget. Each + // newcomer exceeds EDITOR_LARGE_OBJECT, so every insert takes a full accounting pass. + let mut source = Document::new_empty(); + let objects: Vec<_> = (0..40) + .map(|i| { + let mut value = vec![b'y'; 1024 * 1024]; + value[..4].copy_from_slice(&(i as u32).to_le_bytes()); + Object::Ref(source.add(Object::String(crate::PdfString::literal(value)))) + }) + .collect(); + source.update_dict(source.root().unwrap(), |d| d.set(b"Extension".to_vec(), Object::Array(objects))).unwrap(); + let bytes = Arc::new(crate::write_full(&source, &crate::SaveOptions::default()).unwrap()); + let doc = Document::open(bytes).unwrap(); + let root = doc.get(doc.root().unwrap()); + let references = root.as_dict().unwrap().get(b"Extension").unwrap().as_array().unwrap().clone(); + let mut held = None; + for (i, reference) in references.iter().enumerate() { + let Object::Ref(reference) = reference else { panic!("indirect object") }; + let object = doc.get(*reference); + assert_eq!(&object.as_string().unwrap().bytes[..4], &(i as u32).to_le_bytes()); + if i == 0 { + held = Some(object); + } + // Eviction keeps the newcomer. + assert!(doc.cache.lock().unwrap().contains_key(&reference.num), "eviction must keep the newcomer"); + } + assert!(doc.cache.lock().unwrap().len() < references.len(), "the byte budget must evict even below the count cap"); + let last = references[references.len() - 1].as_ref().unwrap(); + assert!(doc.cache.lock().unwrap().contains_key(&last.num), "eviction must keep the newcomer"); + assert_eq!(&held.unwrap().as_string().unwrap().bytes[..4], &0u32.to_le_bytes()); + } + #[test] fn a_stream_limited_document_cannot_be_saved() { let bytes = Arc::new(crate::write_full(&Document::new_empty(), &crate::SaveOptions::default()).unwrap()); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index c2182348..f7aacd40 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -2675,8 +2675,15 @@ impl Session { Scope::Metadata => return Ok(()), Scope::Comments => doc.info.annotations = comment_list(&editor.cos), Scope::Form => { + // PERF: keystroke path, index once. A linear find per field is O(F^2); + // forms reach 10k fields. + let mut by_name = std::collections::HashMap::with_capacity(form.len()); + for x in form.iter() { + // First match wins (do not simplify to collect: it keeps the last). + by_name.entry(x.name.as_str()).or_insert(x); + } for f in &mut doc.info.fields { - if let Some(ff) = form.iter().find(|x| x.name == f.name) { + if let Some(ff) = by_name.get(f.name.as_str()) { f.value = match ff.kind { pdfcraft_forms::FieldKind::CheckBox | pdfcraft_forms::FieldKind::Radio => { Some(ff.value.first().cloned().unwrap_or_else(|| "Off".into())) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index a7446372..79a53189 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1193,6 +1193,37 @@ fn form_edits_refresh_field_values_without_a_full_inspection() { assert_eq!(values(&d.info.fields), [("name".to_string(), Some("Ada".to_string())), ("ok".to_string(), Some("Yes".to_string()))]); } +#[test] +fn form_refresh_keeps_first_value_when_field_names_repeat() { + // Two widgets share one name; the refresh reads the first. + let objs = [ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R 5 0 R 6 0 R] /DA (/Helv 0 Tf 0 g) /DR << /Font << /Helv 7 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 300] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R 6 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (dup) /V (first) /Rect [20 250 180 270] /P 3 0 R >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (dup) /V (second) /Rect [20 220 180 240] /P 3 0 R >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (other) /Rect [20 190 180 210] /P 3 0 R >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + ]; + let mut bytes = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(bytes.len()); + bytes.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = bytes.len(); + bytes.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + bytes.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + bytes.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + let mut s = Session::new(); + let id = s.open("dup.pdf", None, Arc::new(bytes), None).unwrap(); + s.apply(id, Edit::SetFieldValue { name: "other".into(), value: FieldValue::Text("x".into()) }).unwrap(); + let dup = s.get(id).unwrap().info.fields.iter().find(|f| f.name == "dup").unwrap().value.clone(); + assert_eq!(dup.as_deref(), Some("first")); +} + #[test] fn preparing_a_form_adds_renames_and_deletes_fields() { let (mut s, id) = session_with(1); diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 103740b9..2a024655 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -48,6 +48,10 @@ const RESULTS_PER_FRAME: usize = 8; const MAX_THUMB_DEMAND: usize = 256; /// At most this many whole-page rasters are cached beyond the ones on screen. const MAX_CACHED_PAGES: usize = 24; +/// At most this many text layers are cached beyond visible, selected and matched pages. +/// Text layers are glyph structs, far smaller than rasters; deliberately independent of +/// `MAX_CACHED_PAGES` so raster tuning never silently retunes text memory. +const MAX_CACHED_TEXT_PAGES: usize = 24; fn rgba_bytes(width: usize, height: usize) -> usize { width.saturating_mul(height).saturating_mul(4) @@ -1380,11 +1384,39 @@ impl DocView { } self.thumbs.retain(|p, _| admitted.contains(p)); self.stale_thumbs.retain(|p| admitted.contains(p)); + self.retain_text_layers(); // Visible main pages and thumbnails take precedence over background search work. queue.extend(requests.into_iter().filter(|r| r.kind == RequestKind::Text)); queue } + /// Drop text layers nothing can still need. Every visited page's text used to stay + /// cached for the session. Keep visible, selected and matched pages plus the nearest + /// prefetch window; evicted pages re-extract on revisit. (Count window only: unlike + /// rasters there is no byte budget here.) While a search is active keep everything: + /// demand still wants every page, so evicting would re-request evicted pages each + /// frame and churn the workers. Failures are never evicted either: they cost nothing + /// to remember, and evicting one would re-request and re-fail it every frame. + fn retain_text_layers(&mut self) { + if self.texts.len() <= MAX_CACHED_TEXT_PAGES { + return; + } + if self.find.as_ref().is_some_and(|f| !f.case_query.trim().is_empty()) { + return; + } + let mut keep: HashSet = self.frame_visible.clone(); + if let Some(s) = self.selection { + keep.insert(s.page); + } + if let Some(f) = self.find.as_ref() { + keep.extend(f.matches.iter().map(|(p, _)| *p)); + } + let mut rest: Vec = self.texts.keys().filter(|p| !keep.contains(*p)).copied().collect(); + rest.sort_by_key(|p| p.abs_diff(self.current)); + keep.extend(rest.into_iter().take(MAX_CACHED_TEXT_PAGES)); + self.texts.retain(|p, _| keep.contains(p)); + } + pub(crate) fn finish_render_frame(&mut self, ctx: &egui::Context, info: &DocInfo, pool: &RenderPool) { let queue = self.prepare_render_queue(info, ctx.pixels_per_point()); if queue != self.last_queue { @@ -3996,6 +4028,60 @@ trailer << /Root 1 0 R >> DocView::new(DocId(1), &info, ViewDefaults { layout, continuous, ..Default::default() }) } + #[test] + fn text_layers_evict_far_pages_but_keep_visible_and_matches() { + let info = thumbnail_info(100); + let mut v = DocView::new(DocId(1), &info, ViewDefaults::default()); + v.current = 50; + v.frame_visible = HashSet::from([50, 51]); + for p in 0..100 { + v.texts.insert(p, Arc::new(PageText::default())); + } + v.text_failed.insert(0); + v.text_failed.insert(99); + v.find = Some(Find { matches: vec![(0, 0..1)], ..Default::default() }); + let _ = v.prepare_render_queue(&info, 1.0); + assert!(v.texts.contains_key(&50) && v.texts.contains_key(&51), "visible pages stay"); + assert!(v.texts.contains_key(&0), "matched pages stay"); + assert!(v.texts.contains_key(&49), "nearest prefetch window stays"); + assert!(!v.texts.contains_key(&99), "far pages go"); + assert_eq!(v.texts.len(), 2 + 1 + MAX_CACHED_TEXT_PAGES, "visible + matched + prefetch window"); + assert!(v.text_failed.contains(&0) && v.text_failed.contains(&99), "failures are bytes-free: never evict"); + } + + #[test] + fn text_layers_keep_the_selection_page() { + let info = thumbnail_info(100); + let mut v = DocView::new(DocId(1), &info, ViewDefaults::default()); + let text = PageText { + glyphs: vec![pdfcraft_render::TextGlyph { text: "x".into(), rect: [0.0, 0.0, 10.0, 10.0], quad: [[0.0, 0.0]; 4], direction: [1.0, 0.0] }], + line_of: vec![0], + space_before: vec![false], + }; + for p in 0..100 { + v.texts.insert(p, Arc::new(text.clone())); + } + v.select_text(0, 0, 0); + v.current = 90; + v.frame_visible = HashSet::from([90, 91]); + let _ = v.prepare_render_queue(&info, 1.0); + assert_eq!(v.selected_text().as_deref(), Some("x"), "stale selections still copy"); + } + + #[test] + fn text_layers_stay_while_a_search_is_active() { + let info = thumbnail_info(100); + let mut v = DocView::new(DocId(1), &info, ViewDefaults::default()); + v.current = 50; + v.frame_visible = HashSet::from([50]); + for p in 0..100 { + v.texts.insert(p, Arc::new(PageText::default())); + } + v.find = Some(Find { query: "dup".into(), case_query: "dup".into(), matches: vec![(0, 0..1)], ..Default::default() }); + let _ = v.prepare_render_queue(&info, 1.0); + assert_eq!(v.texts.len(), 100, "evicting mid-search would churn the workers"); + } + fn thumbnail_info(count: usize) -> DocInfo { DocInfo { pages: (0..count)