github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-05-19 15:03:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

🔧 Refactor ShapesPool to use index-based storage instead of unsafe lifetime references

Browse Source 
Replace `HashMap<&'a Uuid, ...>` with `HashMap<usize, ...>` for all auxiliary maps
(modifiers, structure, scale_content, modified_shape_cache)
This commit is contained in:
Elena Torro 2026-01-21 14:42:21 +01:00
parent 5b1766835f
commit 5775fa61ba
3 changed files with 127 additions and 224 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
render-wasm/src/main.rs
View File

@ -23,7 +23,7 @@ use std::collections::HashMap;
use utils::uuid_from_u32_quartet; use utils::uuid_from_u32_quartet;
use uuid::Uuid; use uuid::Uuid;
pub(crate) static mut STATE: Option<Box<State<'static>>> = None; pub(crate) static mut STATE: Option<Box<State>> = None;
#[macro_export] #[macro_export]
macro_rules! with_state_mut { macro_rules! with_state_mut {

21
render-wasm/src/state.rs
View File

@ -18,16 +18,16 @@ use crate::shapes::modifiers::grid_layout::grid_cell_data;
/// It is created by [init] and passed to the other exported functions. /// It is created by [init] and passed to the other exported functions.
/// Note that rust-skia data structures are not thread safe, so a state /// Note that rust-skia data structures are not thread safe, so a state
/// must not be shared between different Web Workers. /// must not be shared between different Web Workers.
pub(crate) struct State<'a> { pub(crate) struct State {
pub render_state: RenderState, pub render_state: RenderState,
pub text_editor_state: TextEditorState, pub text_editor_state: TextEditorState,
pub current_id: Option<Uuid>, pub current_id: Option<Uuid>,
pub current_browser: u8, pub current_browser: u8,
pub shapes: ShapesPool<'a>, pub shapes: ShapesPool,
pub saved_shapes: Option<ShapesPool<'a>>, pub saved_shapes: Option<ShapesPool>,
} }
impl<'a> State<'a> { impl State {
pub fn new(width: i32, height: i32) -> Self { pub fn new(width: i32, height: i32) -> Self {
State { State {
render_state: RenderState::new(width, height), render_state: RenderState::new(width, height),
@ -224,16 +224,9 @@ impl<'a> State<'a> {
} }
pub fn rebuild_modifier_tiles(&mut self, ids: Vec<Uuid>) { pub fn rebuild_modifier_tiles(&mut self, ids: Vec<Uuid>) {
// SAFETY: We're extending the lifetime of the mutable borrow to 'a. // No longer need unsafe lifetime extension - index-based storage is safe
// This is safe because: self.render_state
// 1. shapes has lifetime 'a in the struct .rebuild_modifier_tiles(&mut self.shapes, ids);
// 2. The reference won't outlive the struct
// 3. No other references to shapes exist during this call
unsafe {
let shapes_ptr = &mut self.shapes as *mut ShapesPool<'a>;
self.render_state
.rebuild_modifier_tiles(&mut *shapes_ptr, ids);
}
} }
pub fn font_collection(&self) -> &FontCollection { pub fn font_collection(&self) -> &FontCollection {

328
render-wasm/src/state/shapes_pool.rs
View File

@ -28,29 +28,44 @@ const SHAPES_POOL_ALLOC_MULTIPLIER: f32 = 1.3;
/// Shapes are stored in a `Vec<Shape>`, which keeps the `Shape` instances /// Shapes are stored in a `Vec<Shape>`, which keeps the `Shape` instances
/// in a contiguous memory block. /// in a contiguous memory block.
/// ///
pub struct ShapesPoolImpl<'a> { /// # Index-based Design
///
/// All auxiliary HashMaps (modifiers, structure, scale_content, modified_shape_cache)
/// use `usize` indices instead of `&'a Uuid` references. This eliminates:
/// - Unsafe lifetime extensions
/// - The need for `rebuild_references()` after Vec reallocation
/// - Complex lifetime annotations
///
/// The `uuid_to_idx` HashMap maps `Uuid` (owned) to indices, avoiding lifetime issues.
///
pub struct ShapesPoolImpl {
shapes: Vec<Shape>, shapes: Vec<Shape>,
counter: usize, counter: usize,
shapes_uuid_to_idx: HashMap<&'a Uuid, usize>, /// Maps UUID to index in the shapes Vec. Uses owned Uuid, no lifetime needed.
uuid_to_idx: HashMap<Uuid, usize>,
modified_shape_cache: HashMap<&'a Uuid, OnceCell<Shape>>, /// Cache for modified shapes, keyed by index
modifiers: HashMap<&'a Uuid, skia::Matrix>, modified_shape_cache: HashMap<usize, OnceCell<Shape>>,
structure: HashMap<&'a Uuid, Vec<StructureEntry>>, /// Transform modifiers, keyed by index
scale_content: HashMap<&'a Uuid, f32>, modifiers: HashMap<usize, skia::Matrix>,
/// Structure entries, keyed by index
structure: HashMap<usize, Vec<StructureEntry>>,
/// Scale content values, keyed by index
scale_content: HashMap<usize, f32>,
} }
// Type aliases to avoid writing lifetimes everywhere // Type aliases - no longer need lifetimes!
pub type ShapesPool<'a> = ShapesPoolImpl<'a>; pub type ShapesPool = ShapesPoolImpl;
pub type ShapesPoolRef<'a> = &'a ShapesPoolImpl<'a>; pub type ShapesPoolRef<'a> = &'a ShapesPoolImpl;
pub type ShapesPoolMutRef<'a> = &'a mut ShapesPoolImpl<'a>; pub type ShapesPoolMutRef<'a> = &'a mut ShapesPoolImpl;
impl<'a> ShapesPoolImpl<'a> { impl ShapesPoolImpl {
pub fn new() -> Self { pub fn new() -> Self {
ShapesPoolImpl { ShapesPoolImpl {
shapes: vec![], shapes: vec![],
counter: 0, counter: 0,
shapes_uuid_to_idx: HashMap::default(), uuid_to_idx: HashMap::default(),
modified_shape_cache: HashMap::default(), modified_shape_cache: HashMap::default(),
modifiers: HashMap::default(), modifiers: HashMap::default(),
@ -62,15 +77,14 @@ impl<'a> ShapesPoolImpl<'a> {
pub fn initialize(&mut self, capacity: usize) { pub fn initialize(&mut self, capacity: usize) {
performance::begin_measure!("shapes_pool_initialize"); performance::begin_measure!("shapes_pool_initialize");
self.counter = 0; self.counter = 0;
self.shapes_uuid_to_idx = HashMap::with_capacity(capacity); self.uuid_to_idx = HashMap::with_capacity(capacity);
let additional = capacity as i32 - self.shapes.len() as i32; let additional = capacity as i32 - self.shapes.len() as i32;
if additional <= 0 { if additional <= 0 {
return; return;
} }
// Reserve exact capacity to avoid any future reallocations // Reserve extra capacity to avoid future reallocations
// This is critical because we store &'a Uuid references that would be invalidated
let target_capacity = (capacity as f32 * SHAPES_POOL_ALLOC_MULTIPLIER) as usize; let target_capacity = (capacity as f32 * SHAPES_POOL_ALLOC_MULTIPLIER) as usize;
self.shapes self.shapes
.reserve_exact(target_capacity.saturating_sub(self.shapes.len())); .reserve_exact(target_capacity.saturating_sub(self.shapes.len()));
@ -81,15 +95,13 @@ impl<'a> ShapesPoolImpl<'a> {
} }
pub fn add_shape(&mut self, id: Uuid) -> &mut Shape { pub fn add_shape(&mut self, id: Uuid) -> &mut Shape {
let did_reallocate = if self.counter >= self.shapes.len() { if self.counter >= self.shapes.len() {
// We need more space. Check if we'll need to reallocate the Vec. // We need more space
let current_capacity = self.shapes.capacity(); let current_capacity = self.shapes.capacity();
let additional = (self.shapes.len() as f32 * SHAPES_POOL_ALLOC_MULTIPLIER) as usize; let additional = (self.shapes.len() as f32 * SHAPES_POOL_ALLOC_MULTIPLIER) as usize;
let needed_capacity = self.shapes.len() + additional; let needed_capacity = self.shapes.len() + additional;
let will_reallocate = needed_capacity > current_capacity; if needed_capacity > current_capacity {
if will_reallocate {
// Reserve extra space to minimize future reallocations // Reserve extra space to minimize future reallocations
let extra_reserve = (needed_capacity as f32 * 0.5) as usize; let extra_reserve = (needed_capacity as f32 * 0.5) as usize;
self.shapes self.shapes
@ -98,165 +110,68 @@ impl<'a> ShapesPoolImpl<'a> {
self.shapes self.shapes
.extend(iter::repeat_with(|| Shape::new(Uuid::nil())).take(additional)); .extend(iter::repeat_with(|| Shape::new(Uuid::nil())).take(additional));
}
will_reallocate
} else {
false
};
let idx = self.counter; let idx = self.counter;
let new_shape = &mut self.shapes[idx]; let new_shape = &mut self.shapes[idx];
new_shape.id = id; new_shape.id = id;
// Get a reference to the id field in the shape with lifetime 'a // Simply store the UUID -> index mapping. No unsafe lifetime tricks needed!
// SAFETY: This is safe because: self.uuid_to_idx.insert(id, idx);
// 1. We pre-allocate enough capacity to avoid Vec reallocation
// 2. The shape and its id field won't move within the Vec
// 3. The reference won't outlive the ShapesPoolImpl
let id_ref: &'a Uuid = unsafe { &*(&self.shapes[idx].id as *const Uuid) };
self.shapes_uuid_to_idx.insert(id_ref, idx);
self.counter += 1; self.counter += 1;
// If the Vec reallocated, we need to rebuild all references in the HashMaps
// because the old references point to deallocated memory
if did_reallocate {
self.rebuild_references();
}
&mut self.shapes[idx] &mut self.shapes[idx]
} }
// No longer needed! Index-based storage means no references to rebuild.
/// Rebuilds all &'a Uuid references in the HashMaps after a Vec reallocation. // The old rebuild_references() function has been removed entirely.
/// This is necessary because Vec reallocation invalidates all existing references.
fn rebuild_references(&mut self) {
// Rebuild shapes_uuid_to_idx with fresh references
let mut new_map = HashMap::with_capacity(self.shapes_uuid_to_idx.len());
for (_, idx) in self.shapes_uuid_to_idx.drain() {
let id_ref: &'a Uuid = unsafe { &*(&self.shapes[idx].id as *const Uuid) };
new_map.insert(id_ref, idx);
}
self.shapes_uuid_to_idx = new_map;
// Rebuild modifiers with fresh references
if !self.modifiers.is_empty() {
let old_modifiers: Vec<(Uuid, skia::Matrix)> = self
.modifiers
.drain()
.map(|(uuid_ref, matrix)| (*uuid_ref, matrix))
.collect();
for (uuid, matrix) in old_modifiers {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) {
self.modifiers.insert(uuid_ref, matrix);
}
}
}
// Rebuild structure with fresh references
if !self.structure.is_empty() {
let old_structure: Vec<(Uuid, Vec<StructureEntry>)> = self
.structure
.drain()
.map(|(uuid_ref, entries)| (*uuid_ref, entries))
.collect();
for (uuid, entries) in old_structure {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) {
self.structure.insert(uuid_ref, entries);
}
}
}
// Rebuild scale_content with fresh references
if !self.scale_content.is_empty() {
let old_scale_content: Vec<(Uuid, f32)> = self
.scale_content
.drain()
.map(|(uuid_ref, scale)| (*uuid_ref, scale))
.collect();
for (uuid, scale) in old_scale_content {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) {
self.scale_content.insert(uuid_ref, scale);
}
}
}
// Rebuild modified_shape_cache with fresh references
if !self.modified_shape_cache.is_empty() {
let old_cache: Vec<(Uuid, OnceCell<Shape>)> = self
.modified_shape_cache
.drain()
.map(|(uuid_ref, cell)| (*uuid_ref, cell))
.collect();
for (uuid, cell) in old_cache {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) {
self.modified_shape_cache.insert(uuid_ref, cell);
}
}
}
}
pub fn len(&self) -> usize { pub fn len(&self) -> usize {
self.shapes_uuid_to_idx.len() self.uuid_to_idx.len()
} }
pub fn has(&self, id: &Uuid) -> bool { pub fn has(&self, id: &Uuid) -> bool {
self.shapes_uuid_to_idx.contains_key(&id) self.uuid_to_idx.contains_key(id)
} }
pub fn get_mut(&mut self, id: &Uuid) -> Option<&mut Shape> { pub fn get_mut(&mut self, id: &Uuid) -> Option<&mut Shape> {
let idx = *self.shapes_uuid_to_idx.get(&id)?; let idx = *self.uuid_to_idx.get(id)?;
Some(&mut self.shapes[idx]) Some(&mut self.shapes[idx])
} }
pub fn get(&self, id: &Uuid) -> Option<&'a Shape> { /// Get a shape by UUID. Returns the modified shape if modifiers/structure
let idx = *self.shapes_uuid_to_idx.get(&id)?; /// are applied, otherwise returns the base shape.
pub fn get(&self, id: &Uuid) -> Option<&Shape> {
let idx = *self.uuid_to_idx.get(id)?;
// SAFETY: We're extending the lifetimes to 'a. let shape = &self.shapes[idx];
// This is safe because:
// 1. All internal HashMaps and the shapes Vec have fields with lifetime 'a
// 2. The shape at idx won't be moved or reallocated (pre-allocated Vec)
// 3. The id is stored in shapes[idx].id which has lifetime 'a
// 4. The references won't outlive the ShapesPoolImpl
unsafe {
let shape_ptr = &self.shapes[idx] as *const Shape;
let modifiers_ptr = &self.modifiers as *const HashMap<&'a Uuid, skia::Matrix>;
let structure_ptr = &self.structure as *const HashMap<&'a Uuid, Vec<StructureEntry>>;
let scale_content_ptr = &self.scale_content as *const HashMap<&'a Uuid, f32>;
let cache_ptr = &self.modified_shape_cache as *const HashMap<&'a Uuid, OnceCell<Shape>>;
// Extend the lifetime of id to 'a - safe because it's the same Uuid stored in shapes[idx].id // Check if this shape needs modification (has modifiers, structure changes, or is a bool)
let id_ref: &'a Uuid = &*(id as *const Uuid); let needs_modification = shape.is_bool()
|| self.modifiers.contains_key(&idx)
|| self.structure.contains_key(&idx)
|| self.scale_content.contains_key(&idx);
if (*shape_ptr).is_bool() if needs_modification {
|| (*modifiers_ptr).contains_key(&id_ref) // Check if we have a cached modified version
|| (*structure_ptr).contains_key(&id_ref) if let Some(cell) = self.modified_shape_cache.get(&idx) {
|| (*scale_content_ptr).contains_key(&id_ref) Some(cell.get_or_init(|| {
{ let mut modified_shape =
if let Some(cell) = (*cache_ptr).get(&id_ref) { shape.transformed(self.modifiers.get(&idx), self.structure.get(&idx));
Some(cell.get_or_init(|| {
let mut shape = (*shape_ptr).transformed(
(*modifiers_ptr).get(&id_ref),
(*structure_ptr).get(&id_ref),
);
if self.to_update_bool(&shape) { if self.to_update_bool(&modified_shape) {
math_bools::update_bool_to_path(&mut shape, self); math_bools::update_bool_to_path(&mut modified_shape, self);
} }
if let Some(scale) = (*scale_content_ptr).get(&id_ref) { if let Some(scale) = self.scale_content.get(&idx) {
shape.scale_content(*scale); modified_shape.scale_content(*scale);
} }
shape modified_shape
})) }))
} else {
Some(&*shape_ptr)
}
} else { } else {
Some(&*shape_ptr) Some(shape)
} }
} else {
Some(shape)
} }
} }
@ -275,69 +190,68 @@ impl<'a> ShapesPoolImpl<'a> {
} }
pub fn set_modifiers(&mut self, modifiers: HashMap<Uuid, skia::Matrix>) { pub fn set_modifiers(&mut self, modifiers: HashMap<Uuid, skia::Matrix>) {
// Convert HashMap<Uuid, V> to HashMap<&'a Uuid, V> using references from shapes and // Convert HashMap<Uuid, V> to HashMap<usize, V> using indices
// Initialize the cache cells because later we don't want to have the mutable pointer // Initialize the cache cells for affected shapes
let mut ids = Vec::<Uuid>::new(); let mut ids = Vec::<Uuid>::new();
let mut modifiers_with_idx = HashMap::with_capacity(modifiers.len());
let mut modifiers_with_refs = HashMap::with_capacity(modifiers.len());
for (uuid, matrix) in modifiers { for (uuid, matrix) in modifiers {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) { if let Some(idx) = self.uuid_to_idx.get(&uuid).copied() {
// self.modified_shape_cache.insert(uuid_ref, OnceCell::new()); modifiers_with_idx.insert(idx, matrix);
modifiers_with_refs.insert(uuid_ref, matrix); ids.push(uuid);
ids.push(*uuid_ref);
} }
} }
self.modifiers = modifiers_with_refs; self.modifiers = modifiers_with_idx;
let all_ids = shapes::all_with_ancestors(&ids, self, true); let all_ids = shapes::all_with_ancestors(&ids, self, true);
for uuid in all_ids { for uuid in all_ids {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) { if let Some(idx) = self.uuid_to_idx.get(&uuid).copied() {
self.modified_shape_cache.insert(uuid_ref, OnceCell::new()); self.modified_shape_cache.insert(idx, OnceCell::new());
} }
} }
} }
pub fn set_structure(&mut self, structure: HashMap<Uuid, Vec<StructureEntry>>) { pub fn set_structure(&mut self, structure: HashMap<Uuid, Vec<StructureEntry>>) {
// Convert HashMap<Uuid, V> to HashMap<&'a Uuid, V> using references from shapes and // Convert HashMap<Uuid, V> to HashMap<usize, V> using indices
// Initialize the cache cells because later we don't want to have the mutable pointer // Initialize the cache cells for affected shapes
let mut structure_with_refs = HashMap::with_capacity(structure.len()); let mut structure_with_idx = HashMap::with_capacity(structure.len());
let mut ids = Vec::<Uuid>::new(); let mut ids = Vec::<Uuid>::new();
for (uuid, entries) in structure { for (uuid, entries) in structure {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) { if let Some(idx) = self.uuid_to_idx.get(&uuid).copied() {
structure_with_refs.insert(uuid_ref, entries); structure_with_idx.insert(idx, entries);
ids.push(*uuid_ref); ids.push(uuid);
} }
} }
self.structure = structure_with_refs; self.structure = structure_with_idx;
let all_ids = shapes::all_with_ancestors(&ids, self, true); let all_ids = shapes::all_with_ancestors(&ids, self, true);
for uuid in all_ids { for uuid in all_ids {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) { if let Some(idx) = self.uuid_to_idx.get(&uuid).copied() {
self.modified_shape_cache.insert(uuid_ref, OnceCell::new()); self.modified_shape_cache.insert(idx, OnceCell::new());
} }
} }
} }
pub fn set_scale_content(&mut self, scale_content: HashMap<Uuid, f32>) { pub fn set_scale_content(&mut self, scale_content: HashMap<Uuid, f32>) {
// Convert HashMap<Uuid, V> to HashMap<&'a Uuid, V> using references from shapes and // Convert HashMap<Uuid, V> to HashMap<usize, V> using indices
// Initialize the cache cells because later we don't want to have the mutable pointer // Initialize the cache cells for affected shapes
let mut scale_content_with_refs = HashMap::with_capacity(scale_content.len()); let mut scale_content_with_idx = HashMap::with_capacity(scale_content.len());
let mut ids = Vec::<Uuid>::new(); let mut ids = Vec::<Uuid>::new();
for (uuid, value) in scale_content { for (uuid, value) in scale_content {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) { if let Some(idx) = self.uuid_to_idx.get(&uuid).copied() {
scale_content_with_refs.insert(uuid_ref, value); scale_content_with_idx.insert(idx, value);
ids.push(*uuid_ref); ids.push(uuid);
} }
} }
self.scale_content = scale_content_with_refs; self.scale_content = scale_content_with_idx;
let all_ids = shapes::all_with_ancestors(&ids, self, true); let all_ids = shapes::all_with_ancestors(&ids, self, true);
for uuid in all_ids { for uuid in all_ids {
if let Some(uuid_ref) = self.get_uuid_ref(&uuid) { if let Some(idx) = self.uuid_to_idx.get(&uuid).copied() {
self.modified_shape_cache.insert(uuid_ref, OnceCell::new()); self.modified_shape_cache.insert(idx, OnceCell::new());
} }
} }
} }
@ -349,47 +263,33 @@ impl<'a> ShapesPoolImpl<'a> {
self.scale_content = HashMap::default(); self.scale_content = HashMap::default();
} }
/// Get a reference to the Uuid stored in a shape, if it exists pub fn subtree(&self, id: &Uuid) -> ShapesPoolImpl {
pub fn get_uuid_ref(&self, id: &Uuid) -> Option<&'a Uuid> {
let idx = *self.shapes_uuid_to_idx.get(&id)?;
// SAFETY: We're returning a reference with lifetime 'a to a Uuid stored
// in the shapes Vec. This is safe because the Vec is stable (pre-allocated)
// and won't be reallocated.
unsafe { Some(&*(&self.shapes[idx].id as *const Uuid)) }
}
pub fn subtree(&self, id: &Uuid) -> ShapesPoolImpl<'a> {
let Some(shape) = self.get(id) else { let Some(shape) = self.get(id) else {
panic!("Subtree not found"); panic!("Subtree not found");
}; };
let mut shapes = vec![]; let mut shapes = vec![];
let mut idx = 0; let mut new_idx = 0;
let mut shapes_uuid_to_idx = HashMap::default(); let mut uuid_to_idx = HashMap::default();
for id in shape.all_children_iter(self, true, true) { for child_id in shape.all_children_iter(self, true, true) {
let Some(shape) = self.get(&id) else { let Some(child_shape) = self.get(&child_id) else {
panic!("Not found"); panic!("Not found");
}; };
shapes.push(shape.clone()); shapes.push(child_shape.clone());
uuid_to_idx.insert(child_id, new_idx);
let id_ref: &'a Uuid = unsafe { &*(&self.shapes[idx].id as *const Uuid) }; new_idx += 1;
shapes_uuid_to_idx.insert(id_ref, idx);
idx += 1;
} }
let mut result = ShapesPoolImpl { ShapesPoolImpl {
shapes, shapes,
counter: idx, counter: new_idx,
shapes_uuid_to_idx, uuid_to_idx,
modified_shape_cache: HashMap::default(), modified_shape_cache: HashMap::default(),
modifiers: HashMap::default(), modifiers: HashMap::default(),
structure: HashMap::default(), structure: HashMap::default(),
scale_content: HashMap::default(), scale_content: HashMap::default(),
}; }
result.rebuild_references();
result
} }
fn to_update_bool(&self, shape: &Shape) -> bool { fn to_update_bool(&self, shape: &Shape) -> bool {
@ -398,11 +298,21 @@ impl<'a> ShapesPoolImpl<'a> {
} }
let default = &Matrix::default(); let default = &Matrix::default();
let parent_modifier = self.modifiers.get(&shape.id).unwrap_or(default);
// Get parent modifier by index
let parent_idx = self.uuid_to_idx.get(&shape.id);
let parent_modifier = parent_idx
.and_then(|idx| self.modifiers.get(idx))
.unwrap_or(default);
// Returns true if the transform of any child is different to the parent's // Returns true if the transform of any child is different to the parent's
shape.all_children_iter(self, true, false).any(|id| { shape.all_children_iter(self, true, false).any(|child_id| {
!math::is_close_matrix(parent_modifier, self.modifiers.get(&id).unwrap_or(default)) let child_modifier = self
.uuid_to_idx
.get(&child_id)
.and_then(|idx| self.modifiers.get(idx))
.unwrap_or(default);
!math::is_close_matrix(parent_modifier, child_modifier)
}) })
} }
} }