From 7c8ca0bde80f6c969b4c7c675ae0fe979b87422c Mon Sep 17 00:00:00 2001 From: komo1iddin <45395275+komo1iddin@users.noreply.github.com> Date: Sun, 18 Oct 2026 15:46:56 +0800 Subject: [PATCH] M7: keep the text blocks cache after the document is edited (#2504) The engine caches a page's text blocks for opening a paragraph and applying its edit, but only while the document is unmodified: it was keyed by the source identity, which edits keep. After the first edit the cache never hit again, and the page was grouped again on every call (the inline editor and the panel each call it after an apply). pdfcraft_cos::Document::read_state() returns what the document reads as: its source identity, its trailer and every changed object, compared as allocations that the state keeps alive. A clone or a snapshot restored by undo has the same state; any set, add, free or trailer change makes a different one. The cache is keyed by it, so it hits after edits and never returns another state's blocks. On a page of 120 rows, text_blocks after an edit took 0.215 ms per call and now 0.006 ms (release build, best of three; a throwaway timing test). Tests: a read state matches a clone and not a document changed by set, free or a trailer change, nor another document; after an edit the engine groups the page once and undo and redo get their own blocks. The engine test fails without this change. Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/cos/src/document.rs | 62 ++++++++++++++++++++++++++++++++++++++ crates/cos/src/lib.rs | 2 +- crates/engine/src/lib.rs | 24 ++++++--------- crates/engine/src/tests.rs | 21 +++++++++++++ 4 files changed, 94 insertions(+), 15 deletions(-) diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index a0b1f56ee..f12f539da 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -337,6 +337,40 @@ impl PartialEq for SourceIdentity { impl Eq for SourceIdentity {} +/// What a document reads as: its source ([`SourceIdentity`]), its trailer and every object changed +/// since it was opened. A clone, or a snapshot that undo restores, has the same state, and every +/// change makes a different one. Changed objects are compared as allocations, which the state +/// keeps alive, so a later object can't take an address one of them had. +#[derive(Clone)] +pub struct ReadState { + source: SourceIdentity, + trailer: Dict, + overlay: BTreeMap, +} + +impl ReadState { + /// Whether `doc` reads as this state. + pub fn matches(&self, doc: &Document) -> bool { + self.source == doc.source_identity() + && self.trailer == doc.trailer + && self.overlay.len() == doc.overlay.len() + && self.overlay.iter().zip(&doc.overlay).all(|((a, x), (b, y))| { + a == b + && match (x, y) { + (Slot::Set(g, o), Slot::Set(h, p)) => g == h && Arc::ptr_eq(o, p), + (Slot::Freed(g), Slot::Freed(h)) => g == h, + _ => false, + } + }) + } +} + +impl std::fmt::Debug for ReadState { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ReadState").field("changed_objects", &self.overlay.len()).finish_non_exhaustive() + } +} + #[derive(Clone)] pub struct Document { data: Arc>, @@ -801,6 +835,12 @@ impl Document { SourceIdentity(Arc::downgrade(&self.source_identity)) } + /// What the document reads as now (see [`ReadState`]): unlike [`Self::source_identity`], it + /// changes with every edit, so a value derived from the document can be cached against it. + pub fn read_state(&self) -> ReadState { + ReadState { source: self.source_identity(), trailer: self.trailer.clone(), overlay: self.overlay.clone() } + } + pub fn version(&self) -> &str { &self.version } @@ -1935,6 +1975,28 @@ const MAX_XREF_OBJECTS: usize = 8_388_607; mod tests { use super::*; + #[test] + fn a_read_state_matches_a_clone_and_no_changed_document() { + let mut doc = Document::new_empty(); + let a = doc.add(Object::Int(1)); + let state = doc.read_state(); + let snapshot = doc.clone(); + assert!(state.matches(&doc) && state.matches(&snapshot)); + // The same value set again is still a change: another allocation. + doc.set(a, Object::Int(1)); + assert!(!state.matches(&doc) && state.matches(&snapshot)); + let edited = doc.read_state(); + doc.free(a); + assert!(!edited.matches(&doc)); + let mut trailer_changed = snapshot.clone(); + trailer_changed.trailer_mut().set(b"Info".to_vec(), Object::Null); + assert!(!state.matches(&trailer_changed)); + // Another document's state never matches, even with the same changes. + let mut other = Document::new_empty(); + other.add(Object::Int(1)); + assert!(!other.read_state().matches(&snapshot)); + } + /// Build a classic-xref file from object bodies (object i+1 = bodies[i]). pub(crate) fn build(bodies: &[&str], trailer: &str) -> Vec { let mut out = b"%PDF-1.7\n".to_vec(); diff --git a/crates/cos/src/lib.rs b/crates/cos/src/lib.rs index 213ff341b..152daed2d 100644 --- a/crates/cos/src/lib.rs +++ b/crates/cos/src/lib.rs @@ -22,7 +22,7 @@ mod security; mod writer; pub use bytes::Bytes; -pub use document::{Document, Revision, SourceIdentity, XrefEntry}; +pub use document::{Document, ReadState, Revision, SourceIdentity, XrefEntry}; pub use object::{Dict, MAX_DECODED, Name, ObjRef, Object, PdfString, Stream}; pub use parser::{Lexer, is_whitespace, parse_indirect}; pub use pdfcraft_crypt::{ diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 5fefe9b4b..f86af3de0 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -316,15 +316,15 @@ struct Editor { text_blocks: TextBlocksCache, } -/// One page's text blocks ([`pdfcraft_edit::text_blocks`]) and the unmodified document state they -/// were read from. Opening a paragraph and applying its edit both need them, and grouping a long -/// page is not cheap. Only an unmodified document is cached: its identity changes on every edit, -/// undo and save (each reopens the working file), so a stale entry never matches. +/// One page's text blocks ([`pdfcraft_edit::text_blocks`]) and the document state they were read +/// from. Opening a paragraph and applying its edit both need them, and grouping a long page is not +/// cheap. The state ([`pdfcraft_cos::ReadState`]) changes with every edit and save and returns +/// with undo, so a stale entry never matches, also after the document has been edited. #[derive(Default)] struct TextBlocksCache(std::sync::Mutex>); /// The document state, the page (0-based) and its text blocks. -type CachedTextBlocks = (pdfcraft_cos::SourceIdentity, usize, Arc>); +type CachedTextBlocks = (pdfcraft_cos::ReadState, usize, Arc>); impl Clone for TextBlocksCache { fn clone(&self) -> Self { @@ -336,20 +336,16 @@ impl Clone for TextBlocksCache { impl Editor { /// The text blocks on `page` (0-based), from the cache when the document hasn't changed. fn text_blocks(&self, page: usize) -> Result>, EditError> { - let source = (!self.cos.is_modified()).then(|| self.cos.source_identity()); - if let Some(source) = &source - && let Ok(cache) = self.text_blocks.0.lock() - && let Some((s, p, blocks)) = cache.as_ref() - && s == source + if let Ok(cache) = self.text_blocks.0.lock() + && let Some((state, p, blocks)) = cache.as_ref() && *p == page + && state.matches(&self.cos) { return Ok(blocks.clone()); } let blocks = Arc::new(pdfcraft_edit::text_blocks(&self.cos, page)?); - if let Some(source) = source - && let Ok(mut cache) = self.text_blocks.0.lock() - { - *cache = Some((source, page, blocks.clone())); + if let Ok(mut cache) = self.text_blocks.0.lock() { + *cache = Some((self.cos.read_state(), page, blocks.clone())); } Ok(blocks) } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ea410cafb..61c99bb70 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -5890,3 +5890,24 @@ fn a_phone_photo_used_as_a_custom_stamp_is_embedded_upright() { let rect = pdfcraft_annot::summaries(cos)[0].rect; assert!(((rect[2] - rect[0]) - 20.0).abs() < 0.01 && ((rect[3] - rect[1]) - 30.0).abs() < 0.01, "{rect:?}"); } + +/// Opening a paragraph and applying its edit read the page's text blocks; after an edit they are +/// still read once per document state, and undo and redo never get another state's blocks. +#[test] +fn text_blocks_stay_cached_after_an_edit_and_follow_undo_and_redo() { + let (mut session, id) = session_with(1); + let blocks = |s: &Session| s.get(id).unwrap().editor.as_ref().unwrap().text_blocks(0).unwrap(); + let before = blocks(&session); + assert_eq!(before[0].editor_text, "Page 1"); + let edit = Edit::EditTextBlock { page: 0, block: 0, text: "Edited".into(), style: pdfcraft_edit::BlockStyle::default() }; + session.apply(id, edit).unwrap(); + let edited = blocks(&session); + assert_eq!(edited[0].editor_text, "Edited"); + assert!(Arc::ptr_eq(&edited, &blocks(&session)), "the edited page is grouped once"); + session.undo(id).unwrap(); + assert_eq!(blocks(&session)[0].editor_text, "Page 1"); + session.redo(id).unwrap(); + let redone = blocks(&session); + assert_eq!(redone[0].editor_text, "Edited"); + assert!(Arc::ptr_eq(&redone, &blocks(&session))); +}