mirror of
https://github.com/storytold/photocraft.git
synced 2026-11-11 00:11:46 +00:00
perf(format): reuse save cache ownership safely
Move cached keys and compressed buffers between saves, and compare weak allocation pointers without temporary strong-reference updates. Keep payload budgets and archive bytes unchanged. Allow fallible tile edits after a cached save. Move pixel storage when only weak references remain; use a fallible copy for shared storage and restore the original tile if the copy fails. Cover cache key reuse, edits after saves at every depth, and weak/shared tile ownership. Name the fixed revisions behind the existing timings.
This commit is contained in:
4 files changed
+52
-14
No files matched your search
@@ -316,11 +316,12 @@ struct Collect<'c> {
|
||||
}
|
||||
|
||||
impl Sink for Collect<'_> {
|
||||
/// Reuses hashes by allocation and sample byte order without upgrading weak references.
|
||||
fn tile(&mut self, format: PixelFormat, tile: &Arc<Tile>) -> Hash {
|
||||
let key = (Arc::as_ptr(tile) as usize, format.sample);
|
||||
if let Some(cache) = self.hash_cache.as_deref_mut()
|
||||
&& let Some((w, h)) = cache.get(&key)
|
||||
&& w.upgrade().is_some_and(|t| Arc::ptr_eq(&t, tile))
|
||||
&& std::ptr::eq(w.as_ptr(), Arc::as_ptr(tile))
|
||||
{
|
||||
let h = h.clone();
|
||||
self.tiles.entry(h.clone()).or_insert_with(|| (tile.clone(), format.sample));
|
||||
@@ -334,11 +335,12 @@ impl Sink for Collect<'_> {
|
||||
h
|
||||
}
|
||||
|
||||
/// Reuses hashes for the same allocation without upgrading weak references.
|
||||
fn blob(&mut self, data: &Arc<Vec<u8>>) -> Hash {
|
||||
let key = Arc::as_ptr(data) as usize;
|
||||
if let Some(cache) = self.blob_hash_cache.as_deref_mut()
|
||||
&& let Some((w, h)) = cache.get(&key)
|
||||
&& w.upgrade().is_some_and(|b| Arc::ptr_eq(&b, data))
|
||||
&& std::ptr::eq(w.as_ptr(), Arc::as_ptr(data))
|
||||
{
|
||||
let h = h.clone();
|
||||
self.blobs.entry(h.clone()).or_insert_with(|| data.clone());
|
||||
@@ -815,25 +817,25 @@ impl PcraftWriter {
|
||||
let todo: Vec<(&String, &Object)> = p.objects.iter().filter(|(path, _)| !self.compressed.contains_key(*path)).collect();
|
||||
let mut fresh: HashMap<&String, Arc<Vec<u8>>> = par_compress(&todo)?.into_iter().collect();
|
||||
for (path, obj) in &p.objects {
|
||||
let data = match self.compressed.get(path) {
|
||||
Some(d) => {
|
||||
let (key, data) = match self.compressed.remove_entry(path) {
|
||||
Some((key, data)) => {
|
||||
if matches!(obj, Object::Tile(..)) {
|
||||
stats.tiles_reused += 1;
|
||||
}
|
||||
d.clone()
|
||||
(Some(key), data)
|
||||
}
|
||||
None => {
|
||||
match obj {
|
||||
Object::Tile(..) => stats.tiles_written += 1,
|
||||
Object::Blob(_) => stats.blobs_written += 1,
|
||||
}
|
||||
fresh.remove(path).unwrap_or_else(|| Arc::new(obj.compressed()))
|
||||
(None, fresh.remove(path).unwrap_or_else(|| Arc::new(obj.compressed())))
|
||||
}
|
||||
};
|
||||
z.add(path, &data)?;
|
||||
if data.capacity() <= self.compressed_budget.saturating_sub(cached_bytes) {
|
||||
cached_bytes += data.capacity();
|
||||
next.insert(path.clone(), data);
|
||||
next.insert(key.unwrap_or_else(|| path.clone()), data);
|
||||
}
|
||||
}
|
||||
self.compressed = next;
|
||||
@@ -1031,6 +1033,10 @@ fn compressed_cache_budget_limits_retained_capacity_without_changing_bytes() {
|
||||
doc.layers.push(Layer::new("pixels", LayerContent::Raster(surface)));
|
||||
let mut writer = PcraftWriter::new();
|
||||
let (expected, _) = writer.save_zip(&doc, &SaveOptions::default()).unwrap();
|
||||
let key = writer.compressed.keys().next().unwrap().clone();
|
||||
let key_allocation = writer.compressed.get_key_value(&key).unwrap().0.as_ptr();
|
||||
assert_eq!(writer.save_zip(&doc, &SaveOptions::default()).unwrap().0, expected);
|
||||
assert_eq!(writer.compressed.get_key_value(&key).unwrap().0.as_ptr(), key_allocation);
|
||||
let budget = writer.compressed.values().map(|v| v.capacity()).min().unwrap();
|
||||
writer.set_compressed_cache_budget(budget);
|
||||
assert!(writer.compressed.is_empty());
|
||||
|
||||
@@ -59,7 +59,7 @@ mod tests {
|
||||
let (_, stats) = cache.0.lock().unwrap().save_zip(&doc, &SaveOptions::default()).unwrap();
|
||||
assert_eq!(stats.tiles_reused, stats.tiles_total);
|
||||
assert_eq!(stats.tiles_written, 0);
|
||||
doc.layers[0].surface_mut().unwrap().write_pixel(0, 0, &[0.8, 0.3, 0.1, 1.0]);
|
||||
doc.layers[0].surface_mut().unwrap().try_write_region(photocraft_geom::Rect::new(0, 0, 1, 1), &[0.8, 0.3, 0.1, 1.0]).unwrap();
|
||||
let next = export(&doc, "second.pcraft", &options).unwrap();
|
||||
let mut expected = doc.clone();
|
||||
expected.name = "second.pcraft".into();
|
||||
|
||||
@@ -253,10 +253,20 @@ impl Surface {
|
||||
if !self.tiles.contains_key(&c) {
|
||||
let tile = Arc::new(Tile::try_filled(&self.format, &self.default_pixel)?);
|
||||
self.tiles.insert(c, tile);
|
||||
} else if self.tiles.get(&c).is_some_and(|t| Arc::strong_count(t) > 1) {
|
||||
let original = self.tiles.get(&c).ok_or(AllocationError::SizeOverflow)?;
|
||||
let tile = Arc::new(original.try_clone()?);
|
||||
self.tiles.insert(c, tile);
|
||||
} else if self.tiles.get_mut(&c).is_some_and(|t| Arc::get_mut(t).is_none()) {
|
||||
let original = self.tiles.remove(&c).ok_or(AllocationError::SizeOverflow)?;
|
||||
let tile = match Arc::try_unwrap(original) {
|
||||
// Weak cache references must not force a pixel-buffer copy.
|
||||
Ok(tile) => tile,
|
||||
Err(original) => match original.try_clone() {
|
||||
Ok(tile) => tile,
|
||||
Err(error) => {
|
||||
self.tiles.insert(c, original);
|
||||
return Err(error);
|
||||
}
|
||||
},
|
||||
};
|
||||
self.tiles.insert(c, Arc::new(tile));
|
||||
}
|
||||
self.tiles.get_mut(&c).and_then(Arc::get_mut).ok_or(AllocationError::SizeOverflow)
|
||||
}
|
||||
@@ -1280,6 +1290,23 @@ mod tests {
|
||||
assert_eq!(surface, before);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fallible_tile_edits_move_weakly_cached_pixels_and_copy_shared_pixels() {
|
||||
for fmt in [PixelFormat::RGBA8, PixelFormat::RGBA16, PixelFormat::RGBA32F] {
|
||||
let mut surface = Surface::new(fmt);
|
||||
let coord = TileCoord::new(0, 0);
|
||||
let pixels = surface.try_tile_mut(coord).unwrap().bytes().as_ptr();
|
||||
let weak = Arc::downgrade(surface.tile(coord).unwrap());
|
||||
let tile = surface.try_tile_mut(coord).unwrap();
|
||||
assert_eq!(tile.bytes().as_ptr(), pixels);
|
||||
assert!(weak.upgrade().is_none());
|
||||
let snapshot = surface.clone();
|
||||
surface.try_write_region(Rect::new(0, 0, 1, 1), &[1.0; 4]).unwrap();
|
||||
assert_eq!(snapshot.pixel(0, 0), vec![0.0; 4]);
|
||||
assert_eq!(surface.pixel(0, 0), vec![1.0; 4]);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fill_rect_shares_covered_tiles_and_edits_stay_local() {
|
||||
// A 300000² background used to materialize ~360 GB of tiles (#705).
|
||||
|
||||
@@ -5,6 +5,8 @@ blob identities reuse weakly cached hashes; compressed payload capacity retained
|
||||
saves is capped at 64 MiB. The last cache owner releases it. Other formats ignore the cache,
|
||||
and one-shot exports still work. Save As names, previews, cancellation, atomic writes and
|
||||
saved revisions keep their existing behavior. Tile hash identity includes sample depth.
|
||||
Fallible edits move uniquely owned tile pixels when weak cache references remain; shared
|
||||
pixels keep their copy-on-write behavior.
|
||||
|
||||
The format writer's compressed-cache limit defaults to unbounded for existing callers.
|
||||
The UI selects 64 MiB. This is retained memory that a one-shot save would release; it does
|
||||
@@ -20,9 +22,12 @@ Two upstream PSD designs add authored layered-file coverage. No generated noise
|
||||
The table uses seven alternating pairs after warmup and includes thumbnail/composite
|
||||
rendering, encoding, APFS atomic writes, fsync and rename. Hashing, reload equality and file
|
||||
removal are outside timers. Intervals are paired bootstrap 95% descriptions of these samples.
|
||||
The parent's production path is the same as main. Each small edit changes one real-photo tile.
|
||||
These writer-reuse results compare main `d95fdf31` and the original parent `f64a143c`
|
||||
with `05bd004b`; the original parent's production save path matches that main baseline.
|
||||
Each small edit changes one real-photo tile. The later cache-ownership changes remove
|
||||
redundant allocations and reference-count updates; no separate timing gain is claimed.
|
||||
|
||||
| Document | Operation | Main ms | Parent ms | Retained ms | Change vs parent |
|
||||
| Document | Operation | Main ms | Original parent ms | Writer reuse ms | Change vs original parent |
|
||||
|---|---|---:|---:|---:|---:|
|
||||
| 24 MP foliage, U16 | save-first-disk | 966.0 | 966.0 | 984.9 | +2.0% (-8.5..+2.7) |
|
||||
| 24 MP foliage, U16 | save-repeat-disk | 983.0 | 983.0 | 651.8 | -33.7% (-35.8..-30.9) |
|
||||
|
||||
Reference in new issue
Block a user